Skip to content
This repository was archived by the owner on Nov 9, 2019. It is now read-only.

Refactor poll_oneoff on *nix #137

Merged
merged 8 commits into from
Oct 30, 2019
Merged

Refactor poll_oneoff on *nix #137

merged 8 commits into from
Oct 30, 2019

Conversation

@kubkon
Copy link
Member

@kubkon kubkon commented Oct 21, 2019 •
edited
Loading

This PR introduces a couple of changes/fixes:

  • it annotates log::debug! messages with "host" to differentiate
    between file descriptors stored on the WASI side (aka the wrappers)
    and those managed by the host (aka the wrapped)
  • it fixes poll_oneoff with wasi::EVENTTYPE_FD_READ resulting in EBADF  bytecodealliance/wasmtime#440, i.e., incorrect passing of
    file descriptor to poll_oneoff where currently errenously we
    pass in the wrapper instead of the wrapped value
  • it adds a couple more log::debug! macros calls for easier future
    debugging
  • it refactors poll_oneoff for easier maintenance in the future (hopefully!)

@kubkon kubkon requested a review from sunfishcode October 21, 2019 09:18
This was referenced Oct 21, 2019
Closed
Closed
@kubkon kubkon marked this pull request as ready for review October 23, 2019 19:13
@kubkon
Copy link
Member Author

kubkon commented Oct 23, 2019 •
edited
Loading

OK, this PR is ready for review. Note however that it is (unfortunately) expected to fail as it makes breaking API changes which still need to be addressed in wasmtime. The PR is expected to pass the tests now :-)

Additionally, this PR is blocked by CraneStation/wasi-misc-tests#38 (i.e., it really should include misc_testsuite fast-forward to the latest upstream when CraneStation/wasi-misc-tests#38 lands).

@kubkon kubkon mentioned this pull request Oct 23, 2019
Merged
@kubkon kubkon requested a review from marmistrz October 24, 2019 05:26
@kubkon kubkon mentioned this pull request Oct 24, 2019
Merged
kubkon pushed a commit that referenced this pull request Oct 24, 2019
@kubkon
Update poll_oneoff API
3a374d0 
This commit updates `poll_oneoff`'s API in a potentially least
invasive way. That is, it adds unused `WasiCtx` argument to the
syscall which will be required by #137. I am hopeful that this way
 #137 can pass all tests and hence this commit should aid the review
 process.
kubkon added a commit to bytecodealliance/wasmtime that referenced this pull request Oct 24, 2019
@kubkon
Update poll_oneoff's signature
875eea6 
This PR updates `wasmtime_wasi` crate by adjusting `poll_oneoff`'s
signature to that introduced in `wasi_common` in
CraneStation/wasi-common#137. This change is required in order to
fix #440.
marmistrz
marmistrz suggested changes Oct 24, 2019
View reviewed changes
Copy link
Member

@marmistrz marmistrz left a comment

Choose a reason for hiding this comment

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

I closed #140, let's keep all the refactoring in one PR.

@kubkon
Copy link
Member Author

kubkon commented Oct 24, 2019

I closed #140, let's keep all the refactoring in one PR.

That's my bad at poorly synchronising efforts between the two of us. Apologies!

dunnock
dunnock reviewed Oct 24, 2019
View reviewed changes
@marmistrz
Copy link
Member

marmistrz commented Oct 29, 2019

@kubkon can we have a more descriptive PR title?

@kubkon kubkon changed the title Fixes CraneStation/wasmtime#440 Refactor poll_oneoff on *nix Oct 29, 2019
kubkon and others added 8 commits October 30, 2019 09:38
@kubkon
Fixes bytecodealliance/wasmtime#440
947a230 
This commit introduces a couple of changes/fixes:
* it annotates `log::debug!` messages with "host" to differentiate
  between file descriptors stored on the WASI side (aka the wrappers)
  and those managed by the host (aka the wrapped)
* it fixes bytecodealliance/wasmtime#440, i.e., incorrect passing of
  file descriptor to `poll_oneoff` where currently errenously we
  pass in the wrapper instead of the wrapped value
* it adds a couple more `log::debug!` macros calls for easier future
  debugging
@kubkon
Add partial refactorting to poll_oneoff
e0245ab 
This commit lays the groundwork for more clean up to come in
subsequent commits.
@kubkon
Finalise refactoring of poll_oneoff
1187bda
@kubkon
Fix compilation error on Windows
8ac3657
@kubkon @marmistrz
Address majority of suggestions and refactor
1378901 
Co-authored-by: Marcin Mielniczuk <[email protected]>
@kubkon
Add poll_oneoff test case
d7cca1a
Leave timeout in nanoseconds in ClockEventData
918f7ce 
Instead of converting the timeout value from nanoseconds to
milliseconds in the host-independent impl, move the conversion
to *nix-specific impl as the conversion is currently only warranted
by the POSIX `poll` syscall.
Don't fail immediately on bad descriptor
fc48086 
If the user specifies an invalid descriptor inside a subscription,
don't fail immediately but rather generate an event with the thrown
WASI error code, and continue with the remaining, potentially
correct subscriptions.
@kubkon
Copy link
Member Author

kubkon commented Oct 30, 2019

I think we're in a pretty good shape with this now, so I'm gonna go ahead and merge this. If you guys feel that we should still modify something, or something isn't clear, please feel free to submit an issue, and I'll be happy to fix anything that is remaining :-)

@kubkon kubkon merged commit f3a5186 into CraneStation:master Oct 30, 2019
@kubkon kubkon deleted the poll_oneoff_fixes branch October 30, 2019 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sunfishcode sunfishcode Awaiting requested review from sunfishcode

2 more reviewers

@dunnock dunnock dunnock left review comments

@marmistrz marmistrz marmistrz requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

poll_oneoff with wasi::EVENTTYPE_FD_READ resulting in EBADF

3 participants

@kubkon @marmistrz @dunnock