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

[POSIX, Mac] Refactor BSD-specific values out of POSIXFileSystem #1123

Merged
merged 3 commits into from
May 11, 2019

Conversation

chrisd8088
Copy link
Collaborator

@chrisd8088 chrisd8088 commented May 8, 2019

Along with #1104 (DONE), #1119 (DONE), and #1128, this PR is a prerequisite to the addition of a GVFS.Platform.Linux set of classes in #1125.

In particular, we convert POSIXFileSystem into an abstract class and move the majority of its implementation into MacFileSystem, due to the fact that many of the syscall function signatures and flag values, plus the layout of the struct stat fields, 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 NativeFileReader subclass is also adjusted, and we remove some of its unused constants such as Create (for the O_CREAT flag, which isn't used here).

And we correct the return value of getuid(2) in POSIXPlatform to a 32-bit uint. Note that on both Linux and BSD/Darwin, the uid_t typedef can be checked with the following:

gcc -E -xc -include 'sys/types.h' - </dev/null | grep [^u]uid_t | grep ^typedef

/cc @jrbriggs

kivikakk and others added 2 commits May 8, 2019 13:15
Also adjust the visibility of the members of the NativeFileReader
subclass, and remove some of its unused constants.
Copy link
Contributor

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for this!

@chrisd8088
Copy link
Collaborator Author

Thanks @kivikakk -- and I think I managed to retain you as the primary author, since you did the work here!

@chrisd8088 chrisd8088 changed the title Refactor BSD-specific values out of POSIXFileSystem [Refactor BSD-specific values out of POSIXFileSystem May 9, 2019
@chrisd8088 chrisd8088 changed the title [Refactor BSD-specific values out of POSIXFileSystem [POSIX, Mac] Refactor BSD-specific values out of POSIXFileSystem May 9, 2019
private class NativeFileReader
{
public const int ReadOnly = 0x0000;
public const int WriteOnly = 0x0001;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that WriteOnly and Create were dropped, but they were not used. @wilbaker @nickgra do you know why they were there at all?

Copy link
Collaborator Author

@chrisd8088 chrisd8088 May 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved (with question on chmod)

{
public override void ChangeMode(string path, ushort mode)
{
Chmod(path, mode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this not work on Linux? Is mode_t something other than ushort?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

GVFS/GVFS.Platform.Mac/MacFileSystem.cs Show resolved Hide resolved
@chrisd8088 chrisd8088 merged commit 69ac5f7 into microsoft:master May 11, 2019
@chrisd8088 chrisd8088 deleted the refactor-posix-mac-filesystem branch May 11, 2019 04:45
@jrbriggs jrbriggs added this to the M153 milestone May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants