rvolosatovs opened PR #7148 from rvolosatovs:feat/preview2_udp
to bytecodealliance:main
:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->Closes #7012
rvolosatovs edited PR #7148:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->Closes #7012
For the most part this is just directly adapted from TCP implementation
rvolosatovs updated PR #7148.
rvolosatovs updated PR #7148.
rvolosatovs updated PR #7148.
rvolosatovs updated PR #7148.
rvolosatovs updated PR #7148.
rvolosatovs has marked PR #7148 as ready for review.
rvolosatovs requested alexcrichton for a review on PR #7148.
rvolosatovs requested wasmtime-core-reviewers for a review on PR #7148.
alexcrichton submitted PR review:
Thanks for this!
alexcrichton submitted PR review:
Thanks for this!
alexcrichton created PR review comment:
Given the refactoring in https://github.com/bytecodealliance/wasmtime/pull/7120 could this perhaps be simplified by storing the family in the
UdpSocket
itself on creation?
alexcrichton created PR review comment:
Could this be returned as a trap rather than an unwrap?
alexcrichton created PR review comment:
To avoid the syscall here could the peer address perhaps be stored in the
Connected
state?
alexcrichton created PR review comment:
I think it'd be best to clamp this to something smaller as otherwise the guest could effectively cause the host to reserve infinite memory here
alexcrichton created PR review comment:
I think that this may no longer be true because
pollable
s haev to all be already deleted for the abovedelete_resource
to succeed, so if we've gotten this far it should be guaranteed there are no polls anywhere
alexcrichton created PR review comment:
Could this use
if cfg!(...)
instead of#[cfg]
?
alexcrichton created PR review comment:
I'm a bit confused about the
ConnectReady
state here because once a socket has entered this state guests can callsend
andreceive
and the state isn't updated. This means that a socket in this state is permanently ready, which seems perhaps not intended?
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
Absolutely, I was trying to have as few changes from existing TCP implementation as possible
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
Should I remove the TCP logic around this as well then?
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
Yes, indeed, I'll fix that in TCP and here
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ah I didn't review the TCP implementation as it came in, so I'm not sure as to the source of this. In that sense I'm not sure if what the comment refers to is still relevant nowadays. Could you run a
git blame
to see where it came from and ping the author?
rvolosatovs edited PR review comment.
rvolosatovs updated PR #7148.
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
I believe that's the 2f6e9777cbd by @sunfishcode
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
I just removed the capacity altogether
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
Not sure why this was an
unwrap
in the original implementation (in fact, it would always trigger a panic on errors)I changed this to a
Into::into
conversion
rvolosatovs updated PR #7148.
alexcrichton submitted PR review:
Ok thanks! Let's resolve that question in parallel, and I think this is good to land.
alexcrichton has enabled auto merge for PR #7148.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I believe it's an artifact of an earlier iteration of the polling design, when we had host polls waiting on socket I/O and had to do a
shutdown
to wake them up. We should be able to delete this both here and in the TCP impl now.
rvolosatovs updated PR #7148.
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
I've removed it from both
rvolosatovs updated PR #7148.
alexcrichton submitted PR review.
rvolosatovs updated PR #7148.
alexcrichton submitted PR review.
rvolosatovs updated PR #7148.
alexcrichton submitted PR review.
alexcrichton merged PR #7148.
Last updated: Dec 23 2024 at 12:05 UTC