[POSIX, Mac] Refactor BSD-specific values out of POSIXFileSystem#1123
[POSIX, Mac] Refactor BSD-specific values out of POSIXFileSystem#1123chrisd8088 merged 3 commits intomicrosoft:masterfrom
Conversation
Also adjust the visibility of the members of the NativeFileReader subclass, and remove some of its unused constants.
kivikakk
left a comment
There was a problem hiding this comment.
LGTM, thank you for this!
|
Thanks @kivikakk -- and I think I managed to retain you as the primary author, since you did the work here! |
| private class NativeFileReader | ||
| { | ||
| public const int ReadOnly = 0x0000; | ||
| public const int WriteOnly = 0x0001; |
There was a problem hiding this comment.
As a bit of context, these are mappings of the libc API flags O_WRONLY and O_CREAT, as defined in, e.g., FreeBSD's fcntl.h.
It looks to me as though there was some refactoring of native filesystem methods back in December which resulted in the creation of FastFetch.NativeUnixMethods as well as GVFS.Platform.Mac.MacFileSystem, which was then migrated GVFS.Platform.POSIX.POSIXFileSystem by @kivikakk recently so we could share the common POSIX bits with Linux.
My hunch would be that because some of these flags are used in the FastFetch.NativeUnixMethods code, they were included here by accident.
It's a good question, because it points out that if we want to use FastFetch on Linux, we'll need to refactor FastFetch.NativeUnixMethods, which is currently going to work on BSD/Darwin only as these flags like O_CREAT have different values on Linux, and the fields of struct stat are also very different. I've created an issue in our backlog for that!
wilbaker
left a comment
There was a problem hiding this comment.
Approved (with question on chmod)
| { | ||
| public override void ChangeMode(string path, ushort mode) | ||
| { | ||
| Chmod(path, mode); |
There was a problem hiding this comment.
Why does this not work on Linux? Is mode_t something other than ushort?
There was a problem hiding this comment.
Yes, it's a uint -- so the Chmod DLL import wrapper is different on Linux, and hence ChangeMode() ends up having to be in the OS-specific class rather than the base POSIX class, even though it looks the same. You can see these Linux variants in #1125.
There was a problem hiding this comment.
On Linux, it's a 32-bit int (assuming we're using fairly generic hardware and compiler, of course):
$ gcc -E -xc -include 'sys/types.h' - </dev/null | grep mode_t
typedef unsigned int __mode_t;
typedef __mode_t mode_t;
On Mac:
gcc -E -xc -include 'sys/types.h' - </dev/null | grep mode_t | grep ^typedef
typedef __uint16_t __darwin_mode_t;
typedef __darwin_mode_t mode_t;
Per advice from @wilbaker on PR review.
Along with
#1104(DONE),#1119(DONE), and #1128, this PR is a prerequisite to the addition of aGVFS.Platform.Linuxset of classes in #1125.In particular, we convert
POSIXFileSysteminto an abstract class and move the majority of its implementation intoMacFileSystem, due to the fact that many of the syscall function signatures and flag values, plus the layout of thestruct statfields, differ between BSD/Darwin and Linux. So we need to separate these out from the POSIX-level commonalities between the two.The visibility of the members of the
NativeFileReadersubclass is also adjusted, and we remove some of its unused constants such asCreate(for theO_CREATflag, which isn't used here).And we correct the return value of
getuid(2)inPOSIXPlatformto a 32-bituint. Note that on both Linux and BSD/Darwin, theuid_ttypedefcan be checked with the following:/cc @jrbriggs