Skip to content

Leverage fileapi for opening files on windows #13178

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.

Sign up for GitHub

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

Merged
straight-shoota merged 10 commits into from
Mar 16, 2023
Merged

Leverage fileapi for opening files on windows #13178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a7688c8
Leverage `fileapi` for opening files on windows
Blacksmoke16 Mar 11, 2023
c96bc28
Scope difference in file already existing conflict to windows
Blacksmoke16 Mar 11, 2023
8cedeb6
Pass correct flags to `_open_osfhandle`
Blacksmoke16 Mar 11, 2023
dfc0148
Simplify `posix_to_open_opts` logic
Blacksmoke16 Mar 11, 2023
bce559b
Merge branch 'master' into windows-file-deletion
straight-shoota Mar 11, 2023
3886053
Address PR feedback
Blacksmoke16 Mar 13, 2023
3da1fdf
Leverage `Int#bits_set?`
Blacksmoke16 Mar 13, 2023
3be6087
Move some bindings to proper file
Blacksmoke16 Mar 14, 2023
dd12001
Always open in binary mode
Blacksmoke16 Mar 14, 2023
e3bf5a0
Revert to previous `access` handling logic
Blacksmoke16 Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions spec/std/file_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,15 @@ describe "File" do
end
end

it "deletes an open file" do
with_tempfile("delete-file.txt") do |filename|
file = File.open filename, "w"
File.exists?(file.path).should be_true
file.delete
File.exists?(file.path).should be_false
end
end

it "deletes? a file" do
with_tempfile("delete-file.txt") do |filename|
File.open(filename, "w") { }
Expand Down
2 changes: 1 addition & 1 deletion src/crystal/system/file.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ module Crystal::System::File
end

private def self.error_is_file_exists?(errno)
Errno.value.in?(Errno::EEXIST, WinError::ERROR_ALREADY_EXISTS)
errno.in?(Errno::EEXIST, WinError::ERROR_ALREADY_EXISTS)
end

# Closes the internal file descriptor without notifying libevent.
Expand Down
80 changes: 77 additions & 3 deletions src/crystal/system/win32/file.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,85 @@ 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
Copy link
Contributor

@HertzDevil HertzDevil Mar 14, 2023 •
edited
Loading

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

Copy link
Member Author

@Blacksmoke16 Blacksmoke16 Mar 14, 2023 •
edited
Loading

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.

access, disposition, attributes = self.posix_to_open_opts flags, perm

fd = LibC._wopen(System.to_wstr(filename), flags, perm)
handle = LibC.CreateFileW(
System.to_wstr(filename),
access,
LibC::DEFAULT_SHARE_MODE, # UNIX semantics
nil,
disposition,
attributes,
LibC::HANDLE.null
)

if handle == LibC::INVALID_HANDLE_VALUE
# Map ERROR_FILE_EXISTS to Errno::EEXIST to avoid changing semantics of other systems
return {-1, WinError.value.error_file_exists? ? Errno::EEXIST : Errno.value}
end

fd = LibC._open_osfhandle handle, flags

if fd == -1
return {-1, Errno.value}
end

# Only binary mode is supported
LibC._setmode fd, LibC::O_BINARY

{fd, Errno::NONE}
end

private def self.posix_to_open_opts(flags : Int32, perm : ::File::Permissions)
access = if flags.bits_set? LibC::O_WRONLY
LibC::GENERIC_WRITE
elsif flags.bits_set? LibC::O_RDWR
LibC::GENERIC_READ | LibC::GENERIC_WRITE
else
LibC::GENERIC_READ
Copy link
Member Author

@Blacksmoke16 Blacksmoke16 Mar 11, 2023

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.

Copy link
Contributor

@HertzDevil HertzDevil Mar 14, 2023

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

end

if flags.bits_set? LibC::O_APPEND
access |= LibC::FILE_APPEND_DATA
end

if flags.bits_set? LibC::O_TRUNC
if flags.bits_set? LibC::O_CREAT
disposition = LibC::CREATE_ALWAYS
else
disposition = LibC::TRUNCATE_EXISTING
end
elsif flags.bits_set? LibC::O_CREAT
if flags.bits_set? LibC::O_EXCL
disposition = LibC::CREATE_NEW
else
disposition = LibC::OPEN_ALWAYS
end
else
disposition = LibC::OPEN_EXISTING
end

attributes = LibC::FILE_ATTRIBUTE_NORMAL
unless perm.owner_write?
attributes |= LibC::FILE_ATTRIBUTE_READONLY
end

if flags.bits_set? LibC::O_TEMPORARY
attributes |= LibC::FILE_FLAG_DELETE_ON_CLOSE | LibC::FILE_ATTRIBUTE_TEMPORARY
access |= LibC::DELETE
end

if flags.bits_set? LibC::O_SHORT_LIVED
attributes |= LibC::FILE_ATTRIBUTE_TEMPORARY
end

if flags.bits_set? LibC::O_SEQUENTIAL
attributes |= LibC::FILE_FLAG_SEQUENTIAL_SCAN
elsif flags.bits_set? LibC::O_RANDOM
attributes |= LibC::FILE_FLAG_RANDOM_ACCESS
end

{fd, fd == -1 ? Errno.value : Errno::NONE}
{access, disposition, attributes}
end

NOT_FOUND_ERRORS = {
Expand Down
21 changes: 18 additions & 3 deletions src/lib_c/x86_64-windows-msvc/c/fileapi.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,30 @@ lib LibC
fun SetFileAttributesW(lpFileName : LPWSTR, dwFileAttributes : DWORD) : BOOL
fun GetFileAttributesExW(lpFileName : LPWSTR, fInfoLevelId : GET_FILEEX_INFO_LEVELS, lpFileInformation : Void*) : BOOL

OPEN_EXISTING = 3
CREATE_NEW = 1
CREATE_ALWAYS = 2
OPEN_EXISTING = 3
OPEN_ALWAYS = 4
TRUNCATE_EXISTING = 5

FILE_ATTRIBUTE_NORMAL = 0x80
FILE_FLAG_BACKUP_SEMANTICS = 0x02000000
FILE_ATTRIBUTE_NORMAL = 0x80
FILE_ATTRIBUTE_TEMPORARY = 0x100

FILE_FLAG_BACKUP_SEMANTICS = 0x02000000
FILE_FLAG_DELETE_ON_CLOSE = 0x04000000
FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000
FILE_FLAG_RANDOM_ACCESS = 0x10000000
FILE_FLAG_SEQUENTIAL_SCAN = 0x08000000

FILE_SHARE_READ = 0x1
FILE_SHARE_WRITE = 0x2
FILE_SHARE_DELETE = 0x4

DEFAULT_SHARE_MODE = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE

GENERIC_READ = 0x80000000
GENERIC_WRITE = 0x40000000

fun CreateFileW(lpFileName : LPWSTR, dwDesiredAccess : DWORD, dwShareMode : DWORD,
lpSecurityAttributes : SECURITY_ATTRIBUTES*, dwCreationDisposition : DWORD,
dwFlagsAndAttributes : DWORD, hTemplateFile : HANDLE) : HANDLE
Expand Down
2 changes: 2 additions & 0 deletions src/lib_c/x86_64-windows-msvc/c/io.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,6 @@ lib LibC
fun _pipe(pfds : Int*, psize : UInt, textmode : Int) : Int
fun _dup2(fd1 : Int, fd2 : Int) : Int
fun _commit(fd : Int) : Int
fun _open_osfhandle(osfhandle : HANDLE, flags : LibC::Int) : LibC::Int
fun _setmode(fd : LibC::Int, mode : LibC::Int) : LibC::Int
end
7 changes: 5 additions & 2 deletions src/lib_c/x86_64-windows-msvc/c/winnt.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ lib LibC
FILE_ATTRIBUTE_READONLY = 0x1
FILE_ATTRIBUTE_REPARSE_POINT = 0x400

FILE_READ_ATTRIBUTES = 0x80
FILE_WRITE_ATTRIBUTES = 0x0100
FILE_APPEND_DATA = 0x00000004

DELETE = 0x00010000
FILE_READ_ATTRIBUTES = 0x80
FILE_WRITE_ATTRIBUTES = 0x0100

# Memory protection constants
PAGE_READWRITE = 0x04
Expand Down