Added support for UF_HIDDEN flag on macOS and FreeBSD (#29323) #34560

davidkaya · 2019-01-11T23:30:22Z

Closes #29323

I am not sure about the change in Interop.Stat.cs whether it should be merged to CoreFX or whether I should revert it here and make a PR with that change to CoreCLR.

cc: @JeremyKuhne @danmosemsft

davidkaya · 2019-01-11T23:38:01Z

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

            // approximation for the validation done by the Win32 function.
            const FileAttributes allValidFlags =
                FileAttributes.Archive | FileAttributes.Compressed | FileAttributes.Device |
                FileAttributes.Directory | FileAttributes.Encrypted | FileAttributes.Hidden |


FileAttributes.Hidden attribute was twice in the list.

davidkaya · 2019-01-12T09:32:19Z

Builds failed because of SIGABRT, I got those during development when I didn't use private bits of CoreCLR (I don't know whether that is intended). So I guess the change in Interop.Stat.cs will have to be done to CoreCLR. Will wait for comment from the team though before I proceed with the change.

stephentoub · 2019-01-14T15:36:44Z

src/Native/Unix/System.Native/pal_io.h

    int64_t BirthTimeNsec; // nanosecond part
    int64_t Dev;       // ID of the device containing the file
    int64_t Ino;       // inode number of the file
+    uint32_t UserFlags; // user defined flags


This is going to be problematic.

Because of how we've moved code around and shared it between the coreclr and corefx repos, some code in System.Private.CoreLib in the coreclr repo depends on System.Native.so/dylib and this type. When we add this field here, we'll end up corrupting data in System.Private.CoreLib until it can be updated accordingly, but that can't happen until these changes are merged. Catch-22.

I see two immediate ways around that:

Temporarily introduce a new set of APIs/structs that this code uses, e.g. Xx2 instead of Xx. Then once everything has flown to coreclr and it's been updated accordingly and those bits consumed back into corefx, the old ones can be deleted. Then repeat the process to rename everything back to the "good" names.

Try to work within the confines of the existing struct as it's defined, e.g. use the Flags field (which we introduced as part of the PAL and so can do with it what we want) to pass the relevant data rather than defining a new UserFlags field.

How would things look if we did (2)? Is it possible? Could it be done in a relatively clean way, or would it be messy? My preference right now would be (2) if it can be done in at all a reasonable way.

cc: @jkotas, @danmosemsft (longer term we may want to rethink where the shared shims live)

x2 instead of Xx.

We do have x2 APIs for this today because of similar type of change some time ago. We can use this to go back to APIs without the 2 suffix.

I see two immediate ways around that:

3rd option would be to first add the field and let the change propagate everywhere. Once the field is everywhere start actually using it.

This is not that different from how any changes on the CoreCLR/CoreFX boundaries need to be staged, e.g. changes of public APIs that are in use by CoreFX have to be often staged in similar way.

3rd option

Ah, right. I like that option, too.

We do have x2 APIs for this today because of similar type of change some time ago. We can use this to go back to APIs without the 2 suffix.

You're right. Let's do that, then.

If I understood it correctly I should extract this change to separate PR and this PR will be merged once it is propagated everywhere, correct?

Yes @davidkaya I believe htat's right. (We didn't notice you were waiting on this q)

Yup, we first add the field, and then add the rest subsequently.

stephentoub · 2019-01-14T15:39:22Z

src/Native/Unix/System.Native/pal_io.c

 #endif
+
+#if HAVE_STAT_FLAGS
+    dst->UserFlags = src->st_flags;


I'm not sure if it applies to UF_HIDDEN, but in the past we've seen a lot of these defines have different values across platforms, which is one of the reasons we have these shims. As such, if we stick with the UserFlags (see my other previous comment about that), we should define a PAL_ version in pal_io.h that has values which match the managed side, regardless of how the native platform defines it, and then here do the translation, e.g.

dst->UserFlags = (src->st_flags & UF_HIDDEN == UF_HIDDEN) ? PAL_UF_HIDDEN : 0;

Do we want to limit the flags only for the UF_HIDDEN one? As I went through the possible flags, I've noticed that you can also set SF_ARCHIVED (0x00010000) in OSX which can be mapped to FileAttributes.Archived and currently it is not. Is it ok to limit it currently only to UF_HIDDEN for now and if e.g SF_ARCHIVED was to be implemented in the future, then the one implementing it should change the implementation?

Is it ok to limit it currently only to UF_HIDDEN for now and if e.g SF_ARCHIVED was to be implemented in the future, then the one implementing it should change the implementation?

Yup. Let's stick with just hidden for now.

stephentoub · 2019-01-14T15:41:12Z

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

+                if ((attributes & FileAttributes.Hidden) != 0 && (_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) != (uint)Interop.Sys.UserFlags.UF_HIDDEN)
+                {
+                    // If Hidden flag is set and cached file status does not have the flag set then set it
+                    Interop.Sys.LChflags(path, (uint)Interop.Sys.UserFlags.UF_HIDDEN);


Is it ok that we're ignoring errors? Assuming yes, a comment to that effect (saying why) would be good.

I checked the Windows implementation of SetAttributes and it throws when the operation returned != 0 code

corefx/src/System.IO.FileSystem/src/System/IO/FileSystem.Windows.cs

Lines 569 to 575 in 7709675

if (!Interop.Kernel32.SetFileAttributes(fullPath, (int)attributes))

{

int errorCode = Marshal.GetLastWin32Error();

if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER)

throw new ArgumentException(SR.Arg_InvalidFileAttrs, nameof(attributes));

throw Win32Marshal.GetExceptionForWin32Error(errorCode, fullPath);

}

also Unix one throws

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

Line 147 in 4fd3d39

Interop.CheckIo(Interop.Sys.ChMod(path, newMode), path, InitiallyDirectory);

I will wrap it in the Interop.CheckIo to match the existing unix implementation which uses Interop.ChMod.

Sounds good.

stephentoub · 2019-01-14T15:43:46Z

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

+                    // If Hidden flag is set and cached file status does not have the flag set then set it
+                    Interop.Sys.LChflags(path, (uint)Interop.Sys.UserFlags.UF_HIDDEN);
+                }
+                else if ((attributes & FileAttributes.Hidden) == 0 && (_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) == (uint)Interop.Sys.UserFlags.UF_HIDDEN)


Seems like this would be better structured as:

if ((attributes & FileAttributes.Hidden) != 0) { if (_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) != (uint)Interop.Sys.UserFlags.UF_HIDDEN) { ... } } else { if (_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) == (uint)Interop.Sys.UserFlags.UF_HIDDEN) { } }

otherwise we're unnecessarily doing the attributes & Hidden test again for the else block if the if condition is true in the first clause but false in the second. Plus it avoids needing to write the same condition check (one inverted) twice.

stephentoub · 2019-01-14T15:44:12Z

Thanks for working on this.

davidkaya · 2019-01-17T20:50:19Z

src/Common/src/CoreLib/Interop/Unix/System.Native/Interop.Stat.cs

            internal long BirthTimeNsec;
            internal long Dev;
            internal long Ino;
+            internal uint UserFlags;


@stephentoub Should I use the new UserFlags enum here or leave it as uint?

Let's stick with uint.

davidkaya · 2019-01-17T20:51:10Z

@stephentoub made changes based on review, I don't know which changes I should extract to separate PR, could you help me out with that?

Only src/Common/src/CoreLib/Interop/** or everything in src/Common/** && src/Native/**?

danmoseley · 2019-02-11T19:41:23Z

@stephentoub there are some questions for you above it looks like.

karelz · 2019-03-04T05:06:31Z

@stephentoub ping?
@JeremyKuhne can you please help push it forward? It's almost 2 months old PR with long non-activity time (slipped through the cracks).
cc @wfurt for Mac & FreeBSD expertise

davidkaya · 2019-03-05T08:20:14Z

@stephentoub ping?
@JeremyKuhne can you please help push it forward? It's almost 2 months old PR with long non-activity time (slipped through the cracks).
cc @wfurt for Mac & FreeBSD expertise

I will incorporate changes as soon as I get some feedback :)

wfurt · 2019-03-05T21:29:25Z

/azp run

azure-pipelines · 2019-03-05T21:30:03Z

Azure Pipelines successfully started running 4 pipeline(s).

wfurt · 2019-03-05T21:45:38Z

It seems like the change to pal_io.h should come out first @davidkaya. Since it is ABI breaking change it should come out first before we start writing to that field. If I'm not mistaken, rest can come all together e.g. start using new field when every part of the code base sees updated size.

stephentoub · 2019-03-07T05:44:16Z

Sorry, somehow I managed to accidentally mute this thread and wasn't getting any notifications from it.

danmoseley · 2019-03-07T06:11:52Z

src/System.IO.FileSystem/src/System.IO.FileSystem.csproj

    <Compile Include="$(CommonPath)\Interop\Unix\System.Native\Interop.UTimensat.cs">
      <Link>Common\Interop\Unix\Interop.UTimensat.cs</Link>
    </Compile>
+    <Compile Include="$(CommonPath)\Interop\Unix\System.Native\Interop.LChflags.cs">


Nit...sorted list?

danmoseley · 2019-03-07T06:22:00Z

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

+            {
+                if ((attributes & FileAttributes.Hidden) != 0)
+                {
+                    if ((_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) != (uint)Interop.Sys.UserFlags.UF_HIDDEN)


Is this style terser
if ((_fileStatus.UserFlags &(uint)Interop.Sys.UserFlags.UF_HIDDEN) == 0)

Yep, it might. Will change.

In that case you might also change if ((_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) == (uint)Interop.Sys.UserFlags.UF_HIDDEN) to if ((_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) != 0)

davidkaya · 2019-03-07T07:59:28Z

Incorporated all comments, extracted UserFlags field change to #35851

Also removed UserFlags fields from the cs file.

Will merge master into the branch once the second PR gets merged.

danmoseley · 2019-03-08T18:54:33Z

/azp run

azure-pipelines · 2019-03-08T18:55:11Z

Azure Pipelines successfully started running 4 pipeline(s).

danmoseley · 2019-03-11T03:59:21Z

/azp run

azure-pipelines · 2019-03-11T03:59:53Z

Azure Pipelines successfully started running 4 pipeline(s).

danmoseley · 2019-03-11T05:19:24Z

Tests are clean save unrelated http test

danmoseley · 2019-03-11T05:20:22Z

Any more review feedback?

danmoseley · 2019-03-18T01:36:00Z

@JeremyKuhne @stephentoub @jkotas any other feedback

src/Native/Unix/System.Native/pal_io.c

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

src/System.IO.FileSystem/tests/FileInfo/GetSetAttributes.cs

src/System.IO.FileSystem/tests/Base/FileGetSetAttributes.cs

davidkaya · 2019-03-28T08:10:44Z

Sorry for making the changes based on review this late. Unexpected business trip.

stephentoub

Thank you for working on this.

Fixes mono/mono#17304

Backport of dotnet/corefx#34560 Added HAVE_LCHFLAGS & HAVE_STAT_FLAGS to configure.

Backport of dotnet/corefx#34560 Added HAVE_LCHFLAGS & HAVE_STAT_FLAGS to configure. Commit migrated from mono/mono@0269f7b

) * Added UserFlags field * Added UserFlags Commit migrated from dotnet/corefx@8c66b0f

…29323) (dotnet/corefx#34560) * Added support for UF_HIDDEN flag on macOS and FreeBSD (dotnet/corefx#29323) * Changes based on review * Reverted UserFlags field * Sorted include in csproj * Changed comparison style * Reverted UserFlags field * Fixed parentheses * Changes based on PR review * Add missing newline * Add missing newline Commit migrated from dotnet/corefx@b37f018

Added support for UF_HIDDEN flag on macOS and FreeBSD (#29323)

4fd3d39

davidkaya commented Jan 11, 2019

View reviewed changes

stephentoub reviewed Jan 14, 2019

View reviewed changes

Changes based on review

d407e27

davidkaya commented Jan 17, 2019

View reviewed changes

karelz added the area-System.IO label Mar 4, 2019

karelz assigned davidkaya Mar 4, 2019

karelz assigned stephentoub and JeremyKuhne Mar 4, 2019

danmoseley requested a review from JeremyKuhne March 5, 2019 21:31

danmoseley approved these changes Mar 7, 2019

View reviewed changes

davidkaya added 4 commits March 7, 2019 08:41

Merge branch 'master' into macos-hidden-files-rebase

6bbd820

Reverted UserFlags field

6beab30

Sorted include in csproj

1d722eb

Changed comparison style

923318c

davidkaya mentioned this pull request Mar 7, 2019

Added UserFlags field, unblocks #34560 #35851

Merged

Fixed parentheses

418c0bc

stephentoub reviewed Mar 18, 2019

View reviewed changes

src/Native/Unix/System.Native/pal_io.c Outdated Show resolved Hide resolved

stephentoub reviewed Mar 18, 2019

View reviewed changes

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

stephentoub reviewed Mar 18, 2019

View reviewed changes

src/System.IO.FileSystem/tests/FileInfo/GetSetAttributes.cs Outdated Show resolved Hide resolved

stephentoub reviewed Mar 18, 2019

View reviewed changes

src/System.IO.FileSystem/tests/Base/FileGetSetAttributes.cs Show resolved Hide resolved

Changes based on PR review

4b8a14e

stephentoub approved these changes Mar 28, 2019

View reviewed changes

stephentoub added 2 commits March 28, 2019 09:54

Add missing newline

73ba612

Add missing newline

d1ed282

stephentoub merged commit b37f018 into dotnet:master Mar 28, 2019

karelz added this to the 3.0 milestone Apr 1, 2019

wfurt mentioned this pull request Aug 29, 2019

invalidate state when directory is created #40677

Merged

steveisok mentioned this pull request Oct 30, 2019

System.IO didn't implement Hidden attribute flag on macOS mono/mono#17304

Open

steveisok pushed a commit to steveisok/corefx that referenced this pull request Oct 31, 2019

Backport of dotnet#34560

d994544

Fixes mono/mono#17304

steveisok mentioned this pull request Oct 31, 2019

Backport corefx osx hidden files mono/corefx#370

Merged

akoeplinger pushed a commit to mono/mono that referenced this pull request Nov 18, 2019

Bump corefx to pick up mono/corefx#370 (#17758)

0269f7b

Backport of dotnet/corefx#34560 Added HAVE_LCHFLAGS & HAVE_STAT_FLAGS to configure.

ManickaP pushed a commit to ManickaP/runtime that referenced this pull request Jan 20, 2020

Bump corefx to pick up mono/corefx#370 (mono/mono#17758)

ae3a301

Backport of dotnet/corefx#34560 Added HAVE_LCHFLAGS & HAVE_STAT_FLAGS to configure. Commit migrated from mono/mono@0269f7b

danmoseley mentioned this pull request Jun 2, 2020

FileSystemEntry.Attributes property is not correct on Unix dotnet/runtime#37301

Closed

carlossanlop mentioned this pull request Aug 13, 2020

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

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Added UserFlags field, unblocks dotnet/corefx#34560 (dotnet/corefx#35851

f6c2998

) * Added UserFlags field * Added UserFlags Commit migrated from dotnet/corefx@8c66b0f

tmds mentioned this pull request May 4, 2022

[API Implementation]: SafeFileHandle overloads for SetCreationTime, SetLastAccessTime, SetLastWriteTime dotnet/runtime#60507

Merged

5 tasks

	if (!Interop.Kernel32.SetFileAttributes(fullPath, (int)attributes))
	{
	int errorCode = Marshal.GetLastWin32Error();
	if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER)
	throw new ArgumentException(SR.Arg_InvalidFileAttrs, nameof(attributes));
	throw Win32Marshal.GetExceptionForWin32Error(errorCode, fullPath);
	}

Added support for UF_HIDDEN flag on macOS and FreeBSD (#29323) #34560

Added support for UF_HIDDEN flag on macOS and FreeBSD (#29323) #34560

Uh oh!

Conversation

davidkaya commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkaya commented Jan 12, 2019

Uh oh!

stephentoub Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkaya Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Jan 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkaya commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Feb 11, 2019

Uh oh!

karelz commented Mar 4, 2019

Uh oh!

davidkaya commented Mar 5, 2019

Uh oh!

wfurt commented Mar 5, 2019

Uh oh!

azure-pipelines bot commented Mar 5, 2019

Uh oh!

wfurt commented Mar 5, 2019

Uh oh!

stephentoub commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkaya commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Mar 8, 2019

Uh oh!

azure-pipelines bot commented Mar 8, 2019

Uh oh!

danmoseley commented Mar 11, 2019

Uh oh!

stephentoub Jan 14, 2019 •

edited

Loading

davidkaya Jan 14, 2019 •

edited

Loading

davidkaya commented Jan 17, 2019 •

edited

Loading

davidkaya commented Mar 7, 2019 •

edited

Loading