Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way #40641

carlossanlop · 2020-08-11T02:10:06Z

These two files do very similar tasks, but when it comes to collect the Hidden attribute, the first one is simpler than the second one:

runtime/src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEntry.Unix.cs

Lines 59 to 67 in bd6cbe3

    
           if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK) 
        
           { 
        
               isSymlink = true; 
        
           } 
        
           else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN) 
        
               && (Interop.Sys.LStat(entry.FullPath, out Interop.Sys.FileStatus linkTargetStatus) >= 0)) 
        
           { 
        
               isSymlink = (linkTargetStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFLNK; 
        
           }

runtime/src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

Lines 81 to 82 in bd6cbe3

    
           if ((_fileStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFLNK) 
        
               attributes |= FileAttributes.ReparsePoint;

This change ensures both collect the Hidden attribute the same way, and makes it more obvious that both methods are closely related.

Also, the ReadOnly attribute was not being collected in the first file, so I added code to do it, as well as a unit test.

I ran all the System.IO.FileSystem unit tests in Ubuntu and in MacOS and they passed. Pending yet to see the CI results.

This fix needs to go into 5.0, or later it could become a breaking change, as explained here.

… the same way

carlossanlop · 2020-08-11T02:18:37Z

src/libraries/System.IO.FileSystem/tests/Enumeration/SkipAttributeTests.cs


-            // Put a period in front to make it hidden on Unix
-            FileInfo fileTwo = new FileInfo(Path.Combine(testDirectory.FullName, "." + GetTestFileName()));
+            FileInfo fileOne = new FileInfo(Path.Combine(testDirectory.FullName, GetTestFileName()));


No changes in this test, just organized some lines for easier understanding.
It also helps to show that this test is affected by my PR.

EDIT: I actually ended up making changes in this file, to split the tests into each platform, since the IsHidden property is not supported in Linux.

carlossanlop · 2020-08-11T16:35:54Z

@iSazonov can you take a look to this PR? I think I still need to add code to throw but I'm not entirely sure exactly where.

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEntry.Unix.cs

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

src/libraries/System.IO.FileSystem/tests/Enumeration/SkipAttributeTests.cs

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

jozkee · 2020-08-12T03:06:42Z

src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs

            if (PlatformDetection.IsWindows)
                fileTwo.Attributes = fileTwo.Attributes | FileAttributes.Hidden;


Shouldn't this also be performed on other platforms?

You pointed out the documentation says it is supported in Linux, Windows and MacOS. Let me debug this unit test in WIndows, MacOS and Unix to see if it's truly supported there, and if so, then the if should be removed, and maybe a separate test should be added for Linux and MacOS (because of the "." prefix in the name, which already hides the file).

So in Linux, if I remove the code that prepends a "." to the filename, and attempt to set FileAttributes.Hidden to fileTwo, the test fails, because
Interop.Sys.CanSetHiddenFlag returns false.
The documentation needs to be updated for Linux.
I'll test MacOS and Windows now.

It works on OSX. I actually found a test in FileSystemEntry which would prevent the user from collecting an enumeration of hidden files. I fixed it now.
Next: Windows.

Works on Windows too.

…s PR suggestions.

…Split existing Skip attribute test into different platforms.

carlossanlop · 2020-08-13T22:49:45Z

@jozkee @iSazonov can you take another look? The new commits include:

Added unit tests for each platform, and split an existing one into platforms: The FileAttributes.Hidden flag is only supported by Windows and MacOS. Linux does not support it, the file needs to be hidden using a dot prefix in the filename.
I found a bug in the FileSystemEntry.IsHidden property, where we were only checking for the dot prefix in the filename, but we also had to check for the IsHidden attribute.
Cleaned the code as suggested in the PR.

carlossanlop · 2020-08-13T22:53:01Z

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEntry.Unix.cs

@@ -143,7 +140,7 @@ public FileAttributes Attributes
        public DateTimeOffset LastAccessTimeUtc => _status.GetLastAccessTime(FullPath, continueOnError: true);
        public DateTimeOffset LastWriteTimeUtc => _status.GetLastWriteTime(FullPath, continueOnError: true);
        public bool IsDirectory => _status.InitiallyDirectory;
-        public bool IsHidden => _directoryEntry.Name[0] == '.';
+        public bool IsHidden => _directoryEntry.Name[0] == '.' || (Attributes & FileAttributes.Hidden) != 0;


Actually, I think this should be:

public bool IsHidden => _directoryEntry.Name[0] == '.' || (Interop.Sys.CanSetHiddenFlag && (Attributes & FileAttributes.Hidden) != 0);

Otherwise, if the user sets the Hidden flag in Linux, it won't really hide the file, but this will return true.
I'm going to test this on Linux.

I added this check too, just in case. I noticed that the Hidden flag cannot be set to the file Attributes in Linux, because the property performs some checks to see if it's possible. But just in case, attempting to read the value should be protected.

The public property was introduced here: c603407

But the change that added the case for UF_HIDDEN should've added this check:
dotnet/corefx#34560

I had a conversation with @jozkee about this and the Interop.Sys.CanSetHiddenFlag check is not necessary, because the Linux P/Invoke will never allow setting it. So I'm reverting the last commit.

Attributes adds FileAttributes.ReadOnly which is irrelevant in this operation.

Suggested change

public bool IsHidden => _directoryEntry.Name[0] == '.' || (Attributes & FileAttributes.Hidden) != 0;

public bool IsHidden => _directoryEntry.Name[0] == '.' || (_initialAttributes & FileAttributes.Hidden) != 0;

Can you please also add a comment on top explaining where the attribute is supported?

Suggestion applied, and comment added.

jozkee · 2020-08-14T00:06:53Z

src/libraries/System.IO.FileSystem/tests/Enumeration/AttributeTests.cs

        {
+            // Put a period in front to make it hidden on Unix
+            IsHiddenAttributeInternal(useDotPrefix: false, useHiddenFlag: true);


Can you also add an OSX test that uses the dot prefix?

The test IsHiddenAttribute_Unix in line 127 runs in OSX too (TestPlatforms.AnyUnix is true in OSX).

iSazonov · 2020-08-14T13:23:34Z

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

+            return IsReadOnly(_fileStatus);
+        }
+
+        internal static bool IsReadOnly(Interop.Sys.FileStatus fileStatus)


No, it needs to be accessed by FileSystemEntry too, which is in the same assembly.

iSazonov · 2020-08-14T13:24:09Z

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

@@ -39,19 +39,23 @@ internal struct FileStatus
        internal bool IsReadOnly(ReadOnlySpan<char> path, bool continueOnError = false)
        {
            EnsureStatInitialized(path, continueOnError);
+            return IsReadOnly(_fileStatus);


Why do we re-read if we have Refresh() for this.

I'm not sure I understand the question.

The functionality of this code has not been changed. I merely moved the contents of the old IsReadOnly method into its own internal helper static method that can be accessed by both FileSystemEntry and FileStatus, to avoid repeating code.

I don't think we should do a Refresh call here, that's what EnsureStatInitialized is for - it will only perform a Refresh if we haven't called it yet.

iSazonov · 2020-08-14T13:26:22Z

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

        {
-            // IMPORTANT: Attribute logic must match the logic in FileSystemEntry
+            return (fileStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR;


It is better move p/invokes from FileSystemEntry.Initialize() in the place and exclude re-reading too.
The same for all attributes.

I talked to @jozkee about this, we can get them moved to a separate helper class, but I would like to do that in a future PR.

…t on top.

iSazonov · 2020-08-15T16:17:04Z

Thanks for fixing this!

What is Preview we get the fix in?

jozkee · 2020-08-15T21:36:30Z

@iSazonov .NET RC1

Reverts "Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way" (commit 6ed1e41) which introduced a perf regression.

…0-rc2 [release/5.0-rc2] Revert #40641

Reverts "Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way" (commit 6ed1e41) which introduced a perf regression. Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com>

Ensure FileStatus and FileSystemEntry IsHidden attribute is retrieved…

aff0182

… the same way

carlossanlop added the area-System.IO label Aug 11, 2020

carlossanlop added this to the 5.0.0 milestone Aug 11, 2020

carlossanlop requested a review from jozkee August 11, 2020 02:10

carlossanlop self-assigned this Aug 11, 2020

carlossanlop commented Aug 11, 2020

View reviewed changes

carlossanlop mentioned this pull request Aug 11, 2020

FileSystemEntry.Attributes property is not correct on Unix #37301

Closed

carlossanlop requested a review from danmoseley August 11, 2020 16:41

iSazonov reviewed Aug 11, 2020

View reviewed changes

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs Show resolved Hide resolved

jozkee reviewed Aug 12, 2020

View reviewed changes

carlossanlop added 2 commits August 13, 2020 15:42

Add missing check in public property FileSystemEntry.IsHidden. Addres…

cfb8175

…s PR suggestions.

Add tests for Hidden and ReadOnly attribute check for each platform. …

ba00f54

…Split existing Skip attribute test into different platforms.

carlossanlop force-pushed the Hidden branch from 050cd48 to ba00f54 Compare August 13, 2020 22:43

carlossanlop commented Aug 13, 2020

View reviewed changes

carlossanlop force-pushed the Hidden branch from e91029c to ba00f54 Compare August 13, 2020 23:29

jozkee reviewed Aug 14, 2020

View reviewed changes

iSazonov reviewed Aug 14, 2020

View reviewed changes

Use _initialAttributes instead of Attributes for IsHidden. Add commen…

a39bf8a

…t on top.

jozkee approved these changes Aug 14, 2020

View reviewed changes

carlossanlop merged commit 6ed1e41 into dotnet:master Aug 14, 2020

carlossanlop deleted the Hidden branch August 14, 2020 20:04

jeffhandley mentioned this pull request Sep 2, 2020

[Unix] Potential performance regression in Directory.EnumerateFiles #41739

Closed

carlossanlop added a commit to carlossanlop/runtime that referenced this pull request Sep 3, 2020

Revert dotnet#40641

8feee81

Reverts "Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way" (commit 6ed1e41) which introduced a perf regression.

carlossanlop mentioned this pull request Sep 3, 2020

Revert #40641 #41817

Merged

github-actions bot pushed a commit that referenced this pull request Sep 3, 2020

Revert #40641

afc2324

Reverts "Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way" (commit 6ed1e41) which introduced a perf regression.

github-actions bot mentioned this pull request Sep 3, 2020

[release/5.0-rc2] Revert #40641 #41820

Merged

carlossanlop added a commit that referenced this pull request Sep 3, 2020

Merge pull request #41820 from dotnet/backport/pr-41817-to-release/5.…

feefe4e

…0-rc2 [release/5.0-rc2] Revert #40641

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way #40641

Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way #40641

carlossanlop commented Aug 11, 2020 •

edited

Loading

carlossanlop Aug 11, 2020 •

edited

Loading

carlossanlop commented Aug 11, 2020 •

edited

Loading

jozkee Aug 12, 2020

carlossanlop Aug 13, 2020 •

edited

Loading

carlossanlop Aug 13, 2020

carlossanlop Aug 13, 2020

carlossanlop Aug 13, 2020

carlossanlop commented Aug 13, 2020

carlossanlop Aug 13, 2020

carlossanlop Aug 13, 2020

carlossanlop Aug 13, 2020

carlossanlop Aug 13, 2020

jozkee Aug 14, 2020

jozkee Aug 14, 2020

carlossanlop Aug 14, 2020

jozkee Aug 14, 2020

carlossanlop Aug 14, 2020

iSazonov Aug 14, 2020

carlossanlop Aug 14, 2020

iSazonov Aug 14, 2020

carlossanlop Aug 14, 2020

iSazonov Aug 14, 2020

carlossanlop Aug 14, 2020

iSazonov commented Aug 15, 2020

jozkee commented Aug 15, 2020

	if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK)
	{
	isSymlink = true;
	}
	else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN)
	&& (Interop.Sys.LStat(entry.FullPath, out Interop.Sys.FileStatus linkTargetStatus) >= 0))
	{
	isSymlink = (linkTargetStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFLNK;
	}

	if ((_fileStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFLNK)
	attributes \|= FileAttributes.ReparsePoint;

		if (PlatformDetection.IsWindows)
		fileTwo.Attributes = fileTwo.Attributes \| FileAttributes.Hidden;

	public bool IsHidden => _directoryEntry.Name[0] == '.' \|\| (Attributes & FileAttributes.Hidden) != 0;
	public bool IsHidden => _directoryEntry.Name[0] == '.' \|\| (_initialAttributes & FileAttributes.Hidden) != 0;

Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way #40641

Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way #40641

Conversation

carlossanlop commented Aug 11, 2020 • edited Loading

carlossanlop Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

carlossanlop commented Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

carlossanlop Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Aug 15, 2020

jozkee commented Aug 15, 2020

carlossanlop commented Aug 11, 2020 •

edited

Loading

carlossanlop Aug 11, 2020 •

edited

Loading

carlossanlop commented Aug 11, 2020 •

edited

Loading

carlossanlop Aug 13, 2020 •

edited

Loading