-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Leverage fileapi
for opening files on windows
#13178
Leverage fileapi
for opening files on windows
#13178
Conversation
elsif flags & LibC::O_RDWR > 0 | ||
LibC::GENERIC_READ | LibC::GENERIC_WRITE | ||
else | ||
LibC::GENERIC_READ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O_RDONLY
's value is actually 0
, so was failing the > 0
or != 0
check, so I just made it the default given that was 0
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows only says:
To specify the file access mode, you must specify either
_O_RDONLY
,_O_RDWR
, or_O_WRONLY
. There's no default value for the access mode.
So this check is fine on Windows, but to be precise, these flags are not bit flags at all. POSIX defines also O_ACCMODE
, to be used like this:
access =
case flags & LibC::O_ACCMODE # unavailable on MSVC
when LibC::O_RDONLY
when LibC::O_WRONLY
when LibC::O_RDWR
when LibC::O_SEARCH # unavailable on most platforms
when LibC::O_EXEC # unavailable on most platforms
else
raise "not allowed"
end
Not sure why CI thinks EDIT: Ran formatter on both latest version and via the built compiler and nothing change so 🤷. |
@@ -26,11 +26,84 @@ module Crystal::System::File | |||
end | |||
|
|||
def self.open(filename : String, flags : Int32, perm : ::File::Permissions) : {LibC::Int, Errno} | |||
flags |= LibC::O_BINARY | LibC::O_NOINHERIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep these mandatory flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensuring O_BINARY
is added should address your below comment.
EDIT: But to make the code cleaner, might just hardcode/inline the _setmode
call.
O_NOINHERIT
seems to be controlled via the bInheritHandle
property of https://learn.microsoft.com/en-us/previous-versions/windows/desktop/legacy/aa379560(v=vs.85) which is the 4th argument to CreateFileW
. However, the documentation also says:
If this parameter is NULL, the handle returned by CreateFile cannot be inherited by any child processes the application may create and the file or device associated with the returned handle gets a default security descriptor.
So given we'd always be applying O_NOINHERIT
, I think we'll be fine as it is.
Follow up of #12475, but with actually passing tests this time 🎉. That PR was from a while ago so was easier for me to just start a new, esp since #12111 caused some conflicts.
This PR leverages an alternate API for opening files on windows to allow for unix semantics of being allowed to delete a file while it is open. Also added a few additional specs and they are both green now. Primarily inspired by how PHP does it via https://github.com/php/php-src/blob/master/win32/ioutil.c#L187.
The pending tempfile spec for nonwritable folder is still a TODO.
Closes #12475
Resolves #12393