Wasip3 file, stdout, tcp, poll and pipe implementations #711
Conversation
cpetig
left a comment
cpetig
left a comment
There was a problem hiding this comment.
There is plenty of code duplication between the wasip2 and wasip3 files, but the functions are different in central regards. We should keep de-duplication in mind after the functionality is more complete.
Ideally progress should be tracked by a test suite, I guess the old p2 test cases should give a good start.
| #endif | ||
| #ifdef __wasip3__ | ||
| // async interfaces | ||
| int (*read3)(void*, void *buf, size_t nbyte, waitable_t *waitable, wasip3_waitable_status_t *out, off_t**); |
There was a problem hiding this comment.
Previously this just returned the stream_u8_t, but since each subsystem has its own (although compatible (?)) stream_read/write function I fused get_write_stream and write. Works for poll, so 🤷
|
Side note: I am quite proud of a working pipe() implementation. |
This struck me as well skimming over things. I originally organized this wasip2-in-one-file and wasip3-in-another file but it might make sense to have a "shared" file and then a read/write file separately. Unsure though, and like you I'm ok determining that after-the-fact if it's best to refactor back into one file. For now because all tests run on all targets I think we'll have reasonable coverage so long as bugfixes come with a test.
Agreed, and my hope is that the
Agreed! This is a great idea!
Sounds reasonable to me yeah, I'm happy either way. I figure none of this will ship until at least all the TODOs are sorted out, so in the interim I don't think the exact state matters much. Would you be amenable to splitting this apart by subsystem? That'll make it easier to review and also ensure that we can have reasonable test coverage along the way. I might recommend dealing with stdio first since that should get, for example, tests related to sleep working first. That'll enable basic tests to start working and then further functionality can be layered on top. Put another way, I think it'd be good to get the minimal PR to get Also, as you're a first-time-contributor to this repo, your workflow runs will need to get approved per-push (I can't change this in repo settings). If you're able to land a small PR to start that'll make future PRs easier as CI runs won't need approval |
2 participants
This is a rebase of alexcrichton#1 after all dependencies landed.
This doesn't contain the run rework, as #710 will take care of that.
Also the expected symbols will be wrong in this initial dump as I separated out several parts.PS: For now I prefer abort() over returning an error on unimplemented functionality as it is much more easy to debug.