Add test case for poll_oneoff syscall
#38
Conversation
|
r? @dunnock |
Good catch, thanks! It probably makes sense to add comments in this PR as it's a minor change, and I can do it without much hassle. Anyhow, if you have any more comments about this PR, please do let me know and shall fix it ASAP :-) |
| ); | ||
| } | ||
|
|
||
| unsafe fn test_stdin_read() { |
There was a problem hiding this comment.
If the parent process closes stdin, it will not be inherited by the child process. As far as I understand, in such case, fd 1 should be closed and poll should return POLLERR.
Even if we don't support closed stdin, this needs a comment at the very least. We could probably try to write to stdin (if an error occurs, the stdin is definitely closed), try polling it (poll should return immediately), read the whole data from stdin, poll again (expecting a timeout)
We could also consider checking if the behavior for stdout and stderr is correct.
There was a problem hiding this comment.
This one is tricky. We don't actually store a raw host fd to the stdin/out/err. It is true though it is possible to remove a WASI fds pointing at the stdin/out/err, it will not close the raw host fds for the process running Wasm.
I'm trying to figure out if a case where a raw host fd to the stdin could be closed for the process running Wasm. Perhaps for now a comment would suffice? @sunfishcode thoughts?
Good idea about testing stdout and stderr. I'll see how difficult it will be to add it to the test case here.
There was a problem hiding this comment.
The shell script executing may have closed the stdin or stdout, for instance. I guess some CIs do it.
There was a problem hiding this comment.
This seems like something that the wasi-misc-tests framework isn't well equipped to handle. There are a variety of things we'd like to test for involving stdin/stdout/stderr, such as what if they're attached to special things like /dev/random, or fifos, or other things, that we can't do from within WASI itself. So while wasi-misc-tests is nice and simple, we may also need support for a test harness like Lucet's, which can set up a custom WasiCtx, run a WASI program with it, and test it from the outside.
There was a problem hiding this comment.
Hmm, that's a good point. How about we compromise a little bit here, and for the moment, test the positive route for stdin/out/err (NB I've already added a smoke test for the latter two), and in the future, when we figure out a better test harness, etc., we'll either augment this test case or create an additional one testing for ill conditions such as if the descriptors are attached to special things, fifos, etc.?
There was a problem hiding this comment.
sounds okay, but please add a comment that we're assuming that stdin/stdout/stderr are open during the testsuite and state it in the README too
There was a problem hiding this comment.
OK, I've now added the comment in the test case.
This commit adds a test case for `poll_oneoff` syscall. In particular, it builds on the excellent test use case provided by @dunnock in their repo [poll_oneoff_tests] (thanks!), and tests: * simple timeout * stdin read with timeout * fd read and fd write polls [poll_oneoff_tests]: https://github.com/dunnock/poll_oneoff_tests
Co-authored-by: Maxim Vorobjov <[email protected]>
src/bin/poll_oneoff.rs
Outdated
| }, | ||
| }, | ||
| ]; | ||
| let out = poll_oneoff_impl(&in_, 1); |
There was a problem hiding this comment.
stdout and stderr should be writable, so as far as I understand, poll should return immediately with both fds. Did you run the tests?
There was a problem hiding this comment.
Yep, that's correct. You were probably looking at the initial draft commit which included by mistake an EVENTTYPE_FD_READ on stdout/err instead of an EVENTTYPE_FD_WRITE. This has now been fixed.
3 participants
This commit adds a test case for
poll_oneoffsyscall. In particular,it builds on the excellent test use case provided by @dunnock in their
repo poll_oneoff_tests (thanks!), and tests: