Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed API for symbolic links #24271

Closed
carlreinke opened this issue Nov 29, 2017 · 153 comments · Fixed by #54253
Closed

Proposed API for symbolic links #24271

carlreinke opened this issue Nov 29, 2017 · 153 comments · Fixed by #54253
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@carlreinke
Copy link
Contributor

carlreinke commented Nov 29, 2017

Edit by @carlossanlop: Revisited API Proposal



Original proposal:

Rationale

The ability to interact with symbolic links (symlinks) in .NET is currently limited to determining that a file is ReparsePoint. This proposed API provides the ability to identify, read, and create symbolic links.

Proposed API

public class Directory
{
    public static DirectoryInfo CreateSymbolicLink(string linkPath, string targetPath);
    public static string GetSymbolicLinkTargetPath(string linkPath);
    public static bool IsSymbolicLink(string path);
}

public class File
{
    public static FileInfo CreateSymbolicLink(string linkPath, string targetPath);
    public static string GetSymbolicLinkTargetPath(string linkPath);
    public static bool IsSymbolicLink(string path);
}

public class FileSystemInfo
{
    public bool IsSymbolicLink { get; }
    public string SymbolicLinkTargetPath { get; }

    public void CreateSymbolicLink(string linkPath);
}

Details

The path returned from GetSymbolicLinkTargetPath(string)/SymbolicLinkTargetPath will be returned exactly as it is stored in the symbolic link. It may reference a non-existent file or directory.

For the purposes of this API, NTFS Junction Points are considered to be like Linux bind mounts and are not considered to be symbolic links.

Updates

  • Move GetSymbolicLinkTargetPath and IsSymbolicLink from Path to Directory, DirectoryInfo, File and FileInfo.
  • Add CreateSymbolicLink.
  • Split from Proposed API for canonical paths #23871.
  • Change path to linkPath where a link file's path is desired.
  • Move the APIs to the FileSystemInfo base class.
@JeremyKuhne
Copy link
Member

Adding support for symbolic links is particularly important now that Windows has changed the default permissions for creating them. (In the Fall Creator's Update you no longer need to be elevated to create them)

MSDN Symbolic Links

One of my concerns is how we introduce support for junctions, mounts, or possibly Mac aliases. Would we want to be more generic here, like GetLinkType(path)? There is an immediate need to filter both Junctions and Symbolic links from other reparse point types on Windows. I'm also wondering if we really want to add this to all of the classes, or just Path. I'm not sure if "link to me" (*Info classes) is that useful?

namespace System.IO
{
    public static class Path
    {
        // Or perhaps GetKnownLinkType()?
        public static LinkType GetLinkType(string path);
        public void CreateSymbolicLink(string linkPath, string targetPath);
    }

    public enum LinkType
    {
         None,
         Symbolic,
         Junction
    }
}

@carlreinke
Copy link
Contributor Author

carlreinke commented Nov 30, 2017

As far as I know, symbolic links are the only link type that is available on all platforms that .NET Core targets, which makes them more useful than other link types. I don't see a lot of value in supporting other link types, but perhaps I'm just being short-sighted.

Windows differentiates between file and directory symbolic links, so you can't just have Path.CreateSymbolicLink(string, string). (The target doesn't need to exist, so you couldn't query the target to see what kind of symbolic link to create.)

The *Info methods are for creating a link at the path of the *Info to a given target path, so "link from me". (Similar to *Info.Create().)

@JeremyKuhne
Copy link
Member

I don't see a lot of value in supporting other link types, but perhaps I'm just being short-sighted.

I'm not expecting creation to become a priority, but identifying Junctions is important (over all the other reparse points, those two need special treatment when enumerating).

The target doesn't need to exist

I didn't even realize that. Is that case useful to support?

The *Info methods are for creating a link at the path of the *Info to a given target path, so "link from me".

I would think "link to" makes more sense. Creating a FileInfo for the target seems backwards from the way I'd expect people to come at this.

@carlreinke
Copy link
Contributor Author

carlreinke commented Nov 30, 2017

The target doesn't need to exist

I didn't even realize that. Is that case useful to support?

If I'm creating an arbitrary directory structure with some links in it, I wouldn't want to have to figure out what order I need to create the file and directories in order to make it work. Unzipping a .zip file with symbolic links in it, for example. (That's hypothetical at this point though — .zip probably doesn't store whether the symlink is to a directory or a file.)

I would think "link to" makes more sense. Creating a FileInfo for the target seems backwards from the way I'd expect people to come at this.

Maybe I'm confused. I would expect that:

  • File.CreateSymbolicLink(string, string) would return a FileInfo for the symbolic link file that was created.
  • FileInfo.CreateSymbolicLink(string) would create a symbolic link file at FileInfo.FullName that targets the path provided in the argument.

@omariom
Copy link
Contributor

omariom commented Dec 1, 2017

How does PowerShell handle links?

@JeremyKuhne
Copy link
Member

JeremyKuhne commented Jan 16, 2018

@carlreinke Sorry I haven't responded, I've been out on an extended vacation. @jhudsoncedaron is also interested in this area and has opened #24655. We should leverage the interest here and try and get a strong proposal together that we can easily clear through API review.

If I'm creating an arbitrary directory structure with some links in it, I wouldn't want to have to figure out what order I need to create the file and directories in order to make it work.

Agreed, thanks for the example.

Maybe I'm confused.

Sorry, terminology is a little weird and I think I was confusing myself. "Create a symbolic link to me" is what I meant, which is what you're describing I think.

@jhudsoncedaron
Copy link

I hate to disappoint you guys but "create a directory structure with some links in it" is a pill on Windows because Windows decided to demand knowing whether the target is a file or a directory at create time.

@JeremyKuhne
Copy link
Member

"create a directory structure with some links in it" is a pill on Windows because Windows decided to demand knowing whether the target is a file or a directory at create time.

Yeah, not sure why that is- I'll take a look and see if I can find any clues, but I suspect it has some compelling legacy reason that no longer applies.

@JeremyKuhne
Copy link
Member

I'm marking this one ready for review with one tweak:

public class DirectoryInfo
{
    // Create a symbolic link at the given path to this directory info
    public void CreateSymbolicLink( string path );
}

public class FileInfo
{
    // Create a symbolic link at the given path to this file info
    public void CreateSymbolicLink( string path );
}

Info classes can be created for non-existent paths. We will allow creating symbolic links at the given path regardless of the info existence if the OS/FileSystem allows it.

@carlreinke
Copy link
Contributor Author

carlreinke commented Jan 18, 2018

It seems to me that it's inconsistent if

  • FileInfo.Create() creates a file at FileInfo.FullPath and
  • FileInfo.CreateSymbolicLink( string ) does not create a (symlink) file at FileInfo.FullPath.

(And similarly for DirectoryInfo.)

@jhudsoncedaron
Copy link

@carlreinke : There's a bunch of other problems with this API scheme leading me to have to abandon hope of correcting it.

@JeremyKuhne
Copy link
Member

It seems to me that it's inconsistent

@carlreinke I don't think it's a problem, but I'll bring it up at the review. I think it is more of an issue to have IsSymbolicLink change values.

There's a bunch of other problems with this API scheme

@jhudsoncedaron Can you please articulate issues with these specific additional API's here so we can consider them.

I spoke with @pjanotti and we believe that we should be using linkPath and targetPath for additional clarity.

I'm updating the main comment to reflect the discussion.

@jhudsoncedaron
Copy link

jhudsoncedaron commented Jan 18, 2018

@JeremyKuhne : It expects the caller to follow links one at a time. To work correctly, the caller must pass an argument to the constructor that specifies whether to get information about the link or the target of the link. It is context sensitive which one the caller would want.

Also, the results of getting one needs to know if its some strange type of file node or not. We probably don't need to handle fifos, block specials, etc. but we at least need to allow directory walking algorithms to know if they encountered one so they can skip over it.

@JeremyKuhne
Copy link
Member

It expects the caller to follow links one at a time.

I'm fine with public static string GetSymbolicLinkTargetPath(string linkPath, bool recurse = false); We'd have to track cycles and should probably throw.

To work correctly, the caller must pass an argument to the constructor that specifies whether to get information about the link or the target of the link.

I don't see why. We currently don't have a constructor that says to follow to the end so they're always information about the given path. You create a file, you have an Info on the file. You create a link, you get an Info to the new link entry. Nothing should have to change.

That said I think it is probably worth adding a convenience constructor that follows links to the final path. Not having it initially, however, shouldn't block this from moving forward. The biggest issue right now is that we have no way to deal with links. Getting the basics in place for the next release to unblock people is super important I think. That window is rapidly closing.

@jhudsoncedaron
Copy link

jhudsoncedaron commented Jan 18, 2018

That said I think it is probably worth adding a convenience constructor that follows links to the final path.

How many times do I have to tell you that won't work? Recursively reading the link is not the same as asking the OS for the link target information.

It's the difference between lstat() and stat(). I already provided the fragment for constructing stat()'s behavior on Windows. Hint: you don't know how many times the OS recurses until it gives up or if readlink() even returned something that can be followed.

We currently don't have a constructor that says to follow to the end so they're always information about the given path

So add one.

@JeremyKuhne
Copy link
Member

Recursively reading the link is not the same as asking the OS for the link target information.

Could you please provide supporting links and/or some code to clarify? I don't see why recursively getting target paths for symbolic links wouldn't give you an actual file path and I'm struggling to find supporting material.

I'll continue to spend time in the near term investigating the various APIs and looking at the implementation internals.

@jhudsoncedaron
Copy link

new FileInfo("/dev/stdin");

This file really does exist on *nix systems and can be opened if you have a handle to it. But it can't be resolved by readlink() recursively.

 ~$ cat | ls -l /dev/stdin
 lrwxrwxrwx 1 root root 15 Oct  9 08:18 /dev/stdin -> /proc/self/fd/0
 ^C
 ~$ cat | ls -l /proc/self/fd/0
 lr-x------ 1 DOMAIN\jhudson DOMAIN\domain users 64 Jan 18 13:01 /proc/self/fd/0 -> pipe:[2166247]
 ^C
 ~$ cat | ls -l /proc/self/fd/pipe*
 ls: cannot access /proc/self/fd/pipe*: No such file or directory
 ^C
 ~$ cat /dev/stdin
 Hi
 Hi
 ~$

This is not the only example, just the easiest found. When you consider that your recursive symbolic link descent doesn't match the kernel's and when you throw in things like sshfs it's quickly best to go ahead and just assume that readlink() is for informational use only.

@JeremyKuhne
Copy link
Member

This is not the only example, just the easiest found.

Thanks, I'll play around with this a bit more and respond to the thread.

@JeremyKuhne
Copy link
Member

@jhudsoncedaron

Why does one get pipe:[...] when piping through cat? While I can replicate your example I also get the following:

~$ ls -l /dev/stdin
lrwxrwxrwx 1 root root 15 Jan 18 15:15 /dev/stdin -> /proc/self/fd/0
~$ ls -l /proc/self/fd/0
lrwx------ 1 jeremy jeremy 0 Jan 18 15:29 /proc/self/fd/0 -> /dev/tty1
~$ ls -l /dev/tty1
crw-rw---- 1 jeremy tty 4, 1 Jan 18 15:15 /dev/tty1
~$ readlink /dev/stdin
/proc/self/fd/0
~$ readlink /proc/self/fd/0
/dev/tty1
~$ readlink /dev/tty1
~$ readlink -f /dev/stdin
/dev/tty1

Working with any of the intermediate paths seems to work fine.

@jhudsoncedaron
Copy link

jhudsoncedaron commented Jan 18, 2018

Because the link is actually to the file by direct reference, and the path displayed is merely the path used to open the file when it was opened (i.e. potentially stale (file moved or deleted out from under you), wrong namespace (chroot() or something more exotic -- note the whole system except for a couple of process is in a chroot() jail these days), or completely irrelevant (pipe()).

Paths in /proc are often passed from process to process to prevent somebody getting way too clever and finding a way to hijack them. This only works because of the fact that /proc symbolic links are directly resolved. This results in the file that was intended even if somebody else renames it out of the way. Therefore, if the API tries to do readlink() itself rather than the direct resolution the security principle is disabled.

We also note that any userspace virtual filesystem can choose to do this.

@JeremyKuhne
Copy link
Member

I don't understand how your example shows that walking can't be done. Running readlink(0) is effectively the same as running readlink(1) in this case, right? If I walk these with readlink I get a usable, direct path at the end.

@jhudsoncedaron
Copy link

jhudsoncedaron commented Jan 19, 2018

One of the tests shows that /proc/self/fd/pipe:[2166247] doesn't exist. Its most-resolved name is /proc/self/fd/0 which can be opened despite being a symbolic link to a path that doesn't exist.

Also, consider the following C#:

    using (var f = new FileStream("/tmp/zxqxvm", FileMode.Create, FileAccess.ReadWrite))
    {
          File.Delete("/tmp/zxqxvm");
          using (var fv2 = new FileStream("/proc/self/fd/" + f.SafeFileHandle.DangerousGetHandle().ToString()))
          {
                /* succeeds */
                if (!new FileInfo("/proc/self/fd/" + f.SafeFileHandle.DangerousGetHandle().ToString()).Target.Exists)
                        throw new Exception("I opened a file but it doesn't exist.");
          }
    }

If this were written as a test case it should pass.

@JeremyKuhne
Copy link
Member

One of the tests shows that /proc/self/fd/pipe:[2166247] doesn't exist. Its most-resolved name is /proc/self/fd/0 which can be opened despite being a symbolic link to a path that doesn't exist.

But where would /proc/self/fd/pipe:[2166247] enter into this API? We'd never give that back, we'd give back /dev/tty1.

If this were written as a test case it should pass.

In the other proposal .Target is meant to open the target of the symlink, which has been deleted. On Windows it will be true, but only because the file handle is still open. As soon as it the handle closed, it goes false. That said, there is definitely a big difference in behavior here. Windows won't let you open the symlink if the target file is marked for deletion. You get access denied. Take the following directory layout and sample code:

01/18/2018  01:07 PM                 4 a
01/18/2018  01:07 PM    <SYMLINK>      b [a]
01/18/2018  01:08 PM    <SYMLINK>      c [b]
01/18/2018  07:30 PM    <SYMLINK>      d [e]
FileInfo fi = new FileInfo(@"F:\test\links\e");
using (var tempfile = new FileStream(fi.FullName, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.Delete | FileShare.ReadWrite))
{
    File.Delete(fi.FullName);
    using (var fs = new FileStream(@"F:\test\links\a", FileMode.Open, FileAccess.Read)) { }
    using (var fs = new FileStream(@"F:\test\links\b", FileMode.Open, FileAccess.Read)) { }
    using (var fs = new FileStream(@"F:\test\links\c", FileMode.Open, FileAccess.Read)) { }

    // Target still exists, but can't be deleted
    fi.Refresh();
    Console.WriteLine($"File {(fi.Exists ? "exists" : "doesn't exist")}.");

    // Access denied if the file is deleted and all of the handles aren't closed
    // using (var fs = new FileStream(@"F:\test\links\d", FileMode.Open, FileAccess.Read)) { }
}

// Target will no longer exist
fi.Refresh();
Console.WriteLine($"File {(fi.Exists ? "exists" : "doesn't exist")}.");

I don't think that you can create a handle on a file marked for deletion on Windows. I might be remembering incorrectly, however. It is an interesting test case for link handling- I'll check how GetFinalPathNameByHandle/etc. deal with a handle opened on the link itself.

@jhudsoncedaron
Copy link

jhudsoncedaron commented Jan 19, 2018

But where would /proc/self/fd/pipe:[2166247] enter into this API? We'd never give that back, we'd give back /dev/tty1.

You must be having trouble reproducing. In this case, stdin was a pipe handle where cat was piped to ls. Try cat | readlink -f /dev/stdin

Windows won't let you open the symlink if the target file is marked for deletion.

That's a property of NTFS. If you fix your share modes to include FileShareDelete in all those opens and try it with a UNC path to a set-top NAS box it will work (or the path might go away immediately leaving you unable to open it).

The general point being is readlink() is for informational use only. If you want to get the information about a link target, use the API to get information about a link target (stat() or GetFileInformationByHandle(). This is guaranteed to resolve the link the exact same way that opening the link path would. If there's no .NET API to get information about the linked target by the underlying native API call but only resolving by its own readlink() calls I can construct case after case where the .NET API generates spurious failures on static filesystems.

@JeremyKuhne
Copy link
Member

Windows won't let you open the symlink if the target file is marked for deletion.

That's a property of NTFS.

It may be, but it is important. If we're suggesting you can find the state of the link target by opening a handle on it and you can't- we'll have to jump through some serious hoops. We already use FindFirstFile as a workaround for this issue in our code. I'm going to try to see if I can use GetFinalPathNameByHandle to implement the same hack- I'll respond back to the thread when I've finished looking at it.

In any case, we can't rely on leaning too heavy on the link info as the only way to get it programmatically on Windows is through DeviceIoControl, which won't fly for WinRT at this point. I still want to expose the data where we can though.

Providing access to file info through handles has been on my bucket list for some time. Providing a static method on FileSystemInfo that will construct a file/directory info is probably doable FileSystemInfo.CreateFromHandle(SafeFileHandle handle), but won't handle the deleted scenario.

Here is what I'm currently thinking:

public class FileSystemInfo
{
    public bool IsSymbolicLink { get; }

    // The string value of the target for symbolic links
    public string SymbolicLinkTarget { get; }

    public void CreateSymbolicLink(string linkPath);
    public static FileSystemInfo CreateFromHandle(SafeFileHandle handle);
}

public class FileInfo
{
    public FileInfo(string fileName, FileSystemInfoFlags flags);

    // this pointer or FileInfo on the target if this info is a SymbolicLink
    public FileInfo TargetInfo { get; }
}

public class DirectoryInfo
{
    public DirectoryInfo(string fileName, FileSystemInfoFlags flags);

    // this pointer or DirectoryInfo on the target if this info is a SymbolicLink
    public DirectoryInfo TargetInfo { get; }
}

[Flags]
public enum FileSystemInfoFlags
{
    // The class will contain information about the final target of symbolic links
    FollowSymbolicLinks = 0x1;

    // Other options in future 
}

@jhudsoncedaron
Copy link

jhudsoncedaron commented Jan 20, 2018

If we're suggesting you can find the state of the link target by opening a handle on it and you can't- we'll have to jump through some serious hoops.

(assuming you mean because it has a pending delete) I was planning on having FileInfo return the status of "you don't have permission to resolve the link target" in this case, same as if the link target referred to a directory you can't traverse. I already use this technique on Windows native code to see through links.

Oh that reminds me. The behavior of new FileInfo("C:\\DirectoryYouHaveNoAccessTo\\somefile.txt") is not documented yet.

public FileInfo(string fileName, FileSystemInfoFlags flags);

Looks good to me.

@jhudsoncedaron
Copy link

So on checking, my intention to follow the behavior for no permissions to directory for a link to a deleted file is actually great.

If you're checking if the file exists because you want to prompt for overwrite (new FileInfo(filename, 0).Exists (or I think File.Exists(filename) does the job). This treats a dangling symbolic link and a link to a file with a pending delete as exists, which is probably what you want.

If you want to do a file type decision ladder (is this a file or directory I was passed), (new FileInfo(filename, FileSystemInfoFlags.FollowSymbolicLinks).Attributes is right barring exotic cases such as it's a persistent fifo. A symbolic link to a file with a pending delete and a dangling symbolic link will both report 0 for attributes resulting in the decision ladder performing no action (since you can't open it this is what you want). Right now, the fallback of calling FindFirstFileEx causes something silly to happen if it does have a pending delete but oh well. The "file" side of the decision will just have to deal with it like it would have to deal with the file disappearing in between the two calls.

If you want to perform a file open operation just open the file first, then if you don't pass FileShare.Delete you don't have to worry about the zombie state in the first place.

@pjanotti pjanotti removed their assignment Oct 8, 2018
@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 27, 2021
@mklement0

This comment has been minimized.

@iSazonov
Copy link
Contributor

Make .CreateAsSymbolicLink() return a FileSystemEntry instance, not FileSystemInfo (see below).

@mklement0 It is ref struct :-(

@tmds
Copy link
Member

tmds commented May 28, 2021

Why do CreateAsSymbolicLink methods return something? I don't expect it to be common to perform more operations once the link is created.

@iSazonov
Copy link
Contributor

Although the hybrid/composite approach may look attractive, it seem is a breaking change and it would be a completely unacceptable. From a practical point of view it is better to have an API that does exactly what is expected and I wouldn't want to confuse the behavior even more.
If we want to process exactly links, it is better to have a separate API for that - IO.SymbolicLink(Info) class in addition to IO.File(Info/IO.Directory(Info) so that expose FileSystemEntry for the link.

@KevinCathcart
Copy link
Contributor

The motivation is my discovery of an existing, ugly asymmetry: ...

So on .NET 5, reading the times from a link give you the times on the link itself, but when you set them, they send up setting the targets? Yikes!

It would be ideal to be (effectively) all hybrid behavior or none at all. (By "effectively" I mean: with the caveats like Delete affecting the link itself, which would be the expected behavior in a hardlink scenario, ignoring that most platforms disallow directory hardlinks). But I don't think we can go in the direction of all hybrid by default, given that that is a pretty significant breaking change. An opt-in option certainly would be possible.

Going in the direction of non-hybrid by default is also a breaking change (from the timestamp setters). I would not be very concerned about fixing the timestamp setters. It is hard justify the current behavior as anything but a bug. While there may be a few users that would be adversely affected by fixing it, I would imagine just as many if not more users would effectively getting a minor bug fixed. (There seems to be a PR in progress for this: #52639).

The whole situation here is complicated, since in many simple cases you really do only care about the the target of the symlink, and a fair few APIs and syscalls do a decent job of allowing you to ignore that something is a symlink. But in other cases it is critical that you know a path is a symlink. There have been security bugs in applications on multiple occasions resulting from confusing a symlink with a normal file or normal directory. This makes me want to lean in the direction of not trying to do additional clever things with symlinks by default beyond the normal "by default opening a symlink opens the target" (in both the file contents, and directory traversal sense of "opening").

@mklement0
Copy link

mklement0 commented May 28, 2021

@tmds:

If needing to modify the properties of a newly created symlink would be common, returning an object representing it would be justified.
However, Since symlink properties - other than the already-specified target path - are usually irrelevant, I agree that there's no good reason to return such an object.

@KevinCathcart, @iSazonov:

It would be ideal to be (effectively) all hybrid behavior or none at all.
But I don't think we can go in the direction of all hybrid by default

Going all-hybrid by default:

  • enables sensible default behavior for what I expect to be the vast majority of use cases: treat a link as a transparent redirection, and operate on the target - as already happens with respect to content operations (reading a file target's content, enumerating a directory target's entries) and half-happens with properties, namely on setting only (the bug discussed).

  • while definitely a breaking change, to me it falls into - to borrow PowerShell's classification - bucket 3, i.e. a change that is unlikely to impact existing code, because the current behavior is unhelpful.

    • The only code I can see being impacted is code that relies on querying timestamps and attributes, but given that a link's own properties are usually irrelevant, that doesn't strike me as likely.

    • (Going all-hybrid would imply nixing Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix #52639 and implementing the opposite logic: both getting and setting properties should then operate on the target.)

The only unfortunate aspect of a hybrid representation is that in order to avoid a truly serious breaking change, the .Attributes value will have to pretend that ReparsePoint is an attribute of the target, so as not to break existing code that uses this attribute to_infer_ that a FileSystemInfo instance is a link (unreliably, on Windows, given that not all reparse points are links). (A kingdom for a separate .IsLink property! If a ReparseTag property is introduced, it would be free to reflect only the target's value, and could serve to disambiguate.)

But in other cases it is critical that you know a path is a symlink.

And for that an .IsLink property is sufficient - plus the ability to obtain a non-hybrid representation of a link itself, if really needed - see below.


@iSazonov:

It is ref struct :-(

Excellent point, I'm sorry I missed that; what drew me to FileSystemEntry was its type-abstract nature, as described in the help topic (emphasis added):

Provides a lower level view of FileSystemInfo [...]

Since FileSystemEntry being a ref struct rules out its use for on-demand construction, let me propose the following:

  • Derive a new class, LinkInfo (or FileSystemLinkInfo) from FileSystemInfo, which is to be a representation of the properties of a file-system link itself, i.e. the non-hybrid counterpart to FileInfo / DirectoryInfo allowing modification of the link's properties itself:

    • This class should only be constructable with a path that (a) exists and (b) is a file-system link.

    • .Exists would reflect the target's existence (which would then apply to all FileSystemInfo-derived types, as proposed).

    • The class needs an .IsDirectoryLink property or method that signals whether the target is a directory, if known (always known on Windows, on Unix only if the (ultimate) target exists).

    • Situationally, the new ResolveLinkTarget() methods would then return LinkInfo instances, namely if the immediate target (returnFinalTarget: false) happens to be another link.

    • Conceivably, it is then sufficient to offer a .LinkTarget method/property (to get the target path string as defined) on LinkInfo only, not on all FileSystemInfo-derived types.

  • As for FileSystemEntry:

    • To avoid a breaking change, introduce a new .ToFileSystemInfo(bool asLink = false) overload, which if requested returns entries that are links as LinkInfo instances.

    • An .IsLink property would still be helpful (but no other link support is necessary).

@iSazonov
Copy link
Contributor

  • Derive a new class, LinkInfo (or FileSystemLinkInfo) from FileSystemInfo

As MSFT team members said it is better to have separate types because there are a lot of reparse points on Windows (and new ones may appear on Unix) and I agreed with this. So we could introduce SymbolicLink(Info). In this case, this class should not have any hybrid behavior and work with the entity itself - change name, target, time, attributes. etc. Then some magical behavior of FileInfo/DirectoryInfo/FileSystemInfo would be more understandable (I still think we need IsSymbolicLink() there). For example, FileSystemInfo.GetTarget() could return SymbolicLinkInfo.

@carlossanlop
Copy link
Member

carlossanlop commented May 28, 2021

Now that the APIs got approved, the discussion is now about additional/incremental improvements for which we already have issues open. So if you don't mind, and for clarity, let's continue the conversation in those issues:

@tmds
Copy link
Member

tmds commented May 31, 2021

public FileSystemInfo? ResolveLinkTarget(bool returnFinalTarget = false); // Rename

Since you've added string? LinkTarget, I think the appropriate default for returnFinalTarget is true.
The argument may even be dropped imo.

public static FileSystemInfo CreateSymbolicLink(string path, string pathToTarget);

As mentioned earlier, the user won't care about this return value. Consider making this return void.

string? LinkTarget { get; }

I still wish this were a method. Not only for consistency with the other properties, but also because now Refresh will probably make an additional syscall for a property the user may not care about.

public bool IsLink { get; }

I don't understand why this is left out.

@iSazonov
Copy link
Contributor

iSazonov commented Jun 1, 2021

public static FileSystemInfo CreateSymbolicLink(string path, string pathToTarget);

As mentioned earlier, the user won't care about this return value. Consider making this return void.

It is the same as existing static API like Directory.CreateDirectory(). It makes no sense to use another pattern.

public bool IsLink { get; }

I don't understand why this is left out.

It is clear as this could be for symbolic links but we need to take into account Windows reparse point tags and in the case it is not yet clear how to present such API.

@mklement0
Copy link

Regarding an IsLink property (which I also think is called for) in relation to the proposed ReparseTag property: Let's continue the discussion at #1908 (comment)

For the sake of completeness: another link-related proposal, in the context of file-system-item enumeration (opt-in/out of recursing into directory links), is #52666

@jhudsoncedaron
Copy link

So on .NET 5, reading the times from a link give you the times on the link itself, but when you set them, they send up setting the targets? Yikes!

I agree, yikes. The native APIs have the same gotchas.

@mklement0
Copy link

mklement0 commented Jun 1, 2021

Yikes for sure, but, as @KevinCathcart has also noted, pending PRs #52639 and #52639 (both by @hamarb123) are planning to fix that.

In other words: This makes FileSystemInfo instances true representations of the underlying link file-system entries, as they mostly already are (aside from the asymmetry that the PRs will fix, there are two - defensible - exceptions: for file links, Create(), as primarily a content operation, will truncate the target; also, on Unix, there is the white lie that a directory symlink itself has a Directory attribute, which isn't technically true).

As desirable as a mostly-hybrid default representation may be, @KevinCathcart has persuasively argued in #52908 (comment) that this would be a serious backward-compatibility concern, so an opt-in via a FileSystemInfo constructor parameter to either (a) get a hybrid representation or (b) the link's target are needed, as discussed in #52908, which is where we should continue.

As for how the WinAPI functions handle symlinks - see Symbolic Link Effects on File Systems Functions.

@iSazonov
Copy link
Contributor

iSazonov commented Jun 1, 2021

public bool IsLink { get; }

I don't understand why this is left out.

It is clear as this could be for symbolic links but we need to take into account Windows reparse point tags and in the case it is not yet clear how to present such API.

As I pointed in #52666 (comment)

I think we should think about behavior. The behavior can be:

  • regular file system
  • like symbolic link

We even renamed PowerShell method to IsReparsePointLikeSymlink to explicitly indicate this fact.

This force we think that if we want any generalization of RPs like IsLink() method we have to add a mask property (LinkReparsePoints?) where collect all RPs whose behavior we want consider like symbolic link. Then IsLink() would use the property to work as expected and users could add new RPs to the property as new RPs are introduced.

@hamarb123
Copy link
Contributor

Please check my comment in #1908 (comment).

@iSazonov
Copy link
Contributor

iSazonov commented Jun 3, 2021

Thinking more about approved (#24271 (comment)) API I am sure it will work well in PowerShell.

We need to make a conclusion about implementation of target resolving since ReparsePointTag list is open and new tags can be introduced:

  • do we want configurable solution?
  • or we can accept locked implementation like PowerShell IsReparsePointLikeSymlink()? If new RPT will be added can we get a fix in servicing?

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Jun 15, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 9, 2021
@joshudson
Copy link
Contributor

So this doesn't trouble me anymore; I will be content to carry around my own static file provider as long as necessary; however a good test for your own symbolic link API would be fixing this: dotnet/aspnetcore#2774 ; the bug is due to calling new FileInfo() and getting back a length of 0 and not serving any data; should be a trivial fix if your symbolic link API is right.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.