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.

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

Jump to bottom

Implement __wasi_fd_fdstat_get for Windows. #557

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Implement __wasi_fd_fdstat_get for Windows. #557

merged 1 commit into from
Nov 26, 2019

Conversation

peterhuene
Copy link
Member

@peterhuene peterhuene commented Nov 13, 2019 •
edited
Loading

This PR fully implements __wasi_fd_fdstat_get on Windows so that
the descriptor flags can be determined.

It does this by calling into NtQueryInformationFile (safe to call from
user mode) to get the open mode and access of the underlying OS handle.

NtQueryInformationFile isn't included in the winapi crate, so it is
manually being linked against.

This PR also fixes several bugs on Windows:

  • Ignore __WASI_FDFLAG_NONBLOCK by not setting FILE_FLAG_OVERLAPPED
    on file handles (the POSIX behavior for O_NONBLOCK on files).
  • Use FILE_FLAG_WRITE_THROUGH for the __WASI_FDFLAG_?SYNC flags.
  • __WASI_FDFLAG_APPEND should disallow FILE_WRITE_DATA access to
    force append-only on write operations.
  • Use GENERIC_READ and GENERIC_WRITE access flags. The
    latter is required when opening a file for truncation.

@peterhuene
Copy link
Member Author

peterhuene commented Nov 13, 2019 •
edited
Loading

Primarily the truncation_rights test was failing due to not using GENERIC_WRITE and opening the file for write for O_TRUNC.

Apparently CreateFile will check dwDesiredAccess against a literal GENERIC_WRITE (a special flag that means "use the generic write access for whatever type of object we're creating") when truncating a file, even through we were passing in the corresponding FILE_GENERIC_WRITE access flags 🤷‍♂ .

@peterhuene
Copy link
Member Author

I'm also working on fd_fdstat_set_flags.

The idea will be to utilize these changes along with ReOpenFile to reopen the file from the existing descriptor, but with the new access / flags while preserving the current file pointer.

kubkon
kubkon approved these changes Nov 13, 2019
View reviewed changes
Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

LGTM! Amazing work, thanks so much! :-)

crates/wasi-common/build.rs Outdated Show resolved Hide resolved
crates/wasi-common/src/sys/windows/host_impl.rs Outdated Show resolved Hide resolved
crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs Show resolved Hide resolved
crates/wasi-common/winx/src/file.rs Show resolved Hide resolved
crates/wasi-common/winx/src/file.rs Outdated Show resolved Hide resolved
@kubkon
Copy link
Member

kubkon commented Nov 13, 2019

Primarily the truncation_rights test was failing due to not using GENERIC_WRITE and opening the file for write for O_TRUNC.

Apparently CreateFile will check dwDesiredAccess against a literal GENERIC_WRITE (a special flag that means "use the generic write access for whatever type of object we're creating") when truncating a file, even through we were passing in the corresponding FILE_GENERIC_WRITE access flags 🤷‍♂ .

Go Windows...

@kubkon kubkon added the wasi:impl Issues pertaining to WASI implementation in Wasmtime label Nov 13, 2019
@kubkon
Copy link
Member

kubkon commented Nov 13, 2019

Oh, and while we're here, if you could rebase to the latest master, that'd be awesome. I've noticed earlier today that even though we're already providing an implementation of fd_readdir, we somehow didn't have the tests enabled for Windows, #560. So that might have caused a tiny conflict between this branch and master.

@peterhuene
Copy link
Member Author

Will do!

marmistrz
marmistrz reviewed Nov 14, 2019
View reviewed changes
crates/wasi-common/src/sys/windows/host_impl.rs Outdated Show resolved Hide resolved
crates/wasi-common/src/sys/windows/host_impl.rs Outdated Show resolved Hide resolved
crates/wasi-common/src/sys/windows/host_impl.rs Outdated Show resolved Hide resolved
crates/wasi-common/src/sys/windows/host_impl.rs Outdated Show resolved Hide resolved
crates/wasi-common/winx/src/file.rs Show resolved Hide resolved
crates/wasi-common/src/hostcalls_impl/fs.rs Outdated Show resolved Hide resolved
@marmistrz marmistrz mentioned this pull request Nov 14, 2019
Closed
@peterhuene peterhuene force-pushed the implement-windows-fdstat-flags branch from 8d38e2e to 3bc87c5 Compare November 15, 2019 02:04
@peterhuene
Copy link
Member Author

I'm expecting CI to fail on Windows until there's clarification on how we expect the truncation_rights test to function (see #570).

@peterhuene peterhuene requested a review from kubkon November 15, 2019 02:20
@peterhuene peterhuene force-pushed the implement-windows-fdstat-flags branch 2 times, most recently from b39a182 to 62c08b9 Compare November 17, 2019 23:35
@kubkon kubkon mentioned this pull request Nov 26, 2019
Merged
@peterhuene peterhuene force-pushed the implement-windows-fdstat-flags branch from 62c08b9 to d2be013 Compare November 26, 2019 18:54
@peterhuene
Copy link
Member Author

This is ready for a review as it should now pass CI.

truncation_rights remains disabled, but with these changes it should be passing once we figure out the correct behavior of the test (see conversation in #570).

@peterhuene peterhuene force-pushed the implement-windows-fdstat-flags branch from d2be013 to 0a98681 Compare November 26, 2019 19:06
@peterhuene
Implement __wasi_fd_fdstat_get for Windows.
45f71e5 
This commit fully implements `__wasi_fd_fdstat_get` on Windows so that
the descriptor flags can be determined.

It does this by calling into `NtQueryInformationFile` (safe to call from
user mode) to get the open mode and access of the underlying OS handle.

`NtQueryInformationFile` isn't included in the `winapi` crate, so it is
manually being linked against.

This commit also fixes several bugs on Windows:

* Ignore `__WASI_FDFLAG_NONBLOCK` by not setting `FILE_FLAG_OVERLAPPED`
  on file handles (the POSIX behavior for `O_NONBLOCK` on files).
* Use `FILE_FLAG_WRITE_THROUGH` for the `__WASI_FDFLAG_?SYNC` flags.
* `__WASI_FDFLAG_APPEND` should disallow `FILE_WRITE_DATA` access to
  force append-only on write operations.
* Use `GENERIC_READ` and `GENERIC_WRITE` access flags.  The
  latter is required when opening a file for truncation.
@peterhuene peterhuene force-pushed the implement-windows-fdstat-flags branch from 0a98681 to 45f71e5 Compare November 26, 2019 19:57
kubkon
kubkon approved these changes Nov 26, 2019
View reviewed changes
Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

LGTM! I'm gonna go ahead and merge this in so that we can progress with other work, leaving the problem of truncation rights for a subsequent PR. Please feel free to file an issue if you find anything amiss though!

@kubkon kubkon merged commit 0cf54ff into bytecodealliance:master Nov 26, 2019
@peterhuene peterhuene deleted the implement-windows-fdstat-flags branch November 26, 2019 21:44
@marmistrz
Copy link
Contributor

I think it was a very good decision to just leave O_TRUNC out and merge it!

@marmistrz marmistrz mentioned this pull request Dec 15, 2019
Merged
@marmistrz marmistrz mentioned this pull request Mar 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marmistrz marmistrz marmistrz left review comments

@kubkon kubkon kubkon approved these changes

@sunfishcode sunfishcode Awaiting requested review from sunfishcode

Assignees
No one assigned
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peterhuene @kubkon @marmistrz