Wasmtime GitHub notifications bot (Oct 04 2023 at 13:48): Wasmtime GitHub notifications bot (Oct 04 2023 at 13:48):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
Wasmtime GitHub notifications bot (Oct 04 2023 at 13:48):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
Wasmtime GitHub notifications bot (Oct 04 2023 at 13:49):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 13:46):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 13:48):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 13:50):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 13:52):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:08):rvolosatovs has marked PR #7148 as ready for review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:09):rvolosatovs requested alexcrichton for a review on PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:09):rvolosatovs requested wasmtime-core-reviewers for a review on PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):alexcrichton submitted PR review:
Thanks for this!
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):alexcrichton submitted PR review:
Thanks for this!
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):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
UdpSocketitself on creation?
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):alexcrichton created PR review comment:
Could this be returned as a trap rather than an unwrap?
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):alexcrichton created PR review comment:
To avoid the syscall here could the peer address perhaps be stored in the
Connectedstate?
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):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
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):alexcrichton created PR review comment:
I think that this may no longer be true because
pollables haev to all be already deleted for the abovedelete_resourceto succeed, so if we've gotten this far it should be guaranteed there are no polls anywhere
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):alexcrichton created PR review comment:
Could this use
if cfg!(...)instead of#[cfg]?
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:41):alexcrichton created PR review comment:
I'm a bit confused about the
ConnectReadystate here because once a socket has entered this state guests can callsendandreceiveand the state isn't updated. This means that a socket in this state is permanently ready, which seems perhaps not intended?
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:51):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:51):rvolosatovs created PR review comment:
Absolutely, I was trying to have as few changes from existing TCP implementation as possible
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:52):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 14:52):rvolosatovs created PR review comment:
Should I remove the TCP logic around this as well then?
Wasmtime GitHub notifications bot (Oct 06 2023 at 15:04):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 15:04):rvolosatovs created PR review comment:
Yes, indeed, I'll fix that in TCP and here
Wasmtime GitHub notifications bot (Oct 06 2023 at 16:40):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 16:40):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 blameto see where it came from and ping the author?
Wasmtime GitHub notifications bot (Oct 06 2023 at 17:32):rvolosatovs edited PR review comment.
Wasmtime GitHub notifications bot (Oct 06 2023 at 17:49):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 17:54):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 17:54):rvolosatovs created PR review comment:
I believe that's the 2f6e9777cbd by @sunfishcode
Wasmtime GitHub notifications bot (Oct 06 2023 at 18:20):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 18:20):rvolosatovs created PR review comment:
I just removed the capacity altogether
Wasmtime GitHub notifications bot (Oct 06 2023 at 18:21):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 18:21):rvolosatovs created PR review comment:
Not sure why this was an
unwrapin the original implementation (in fact, it would always trigger a panic on errors)I changed this to a
Into::intoconversion
Wasmtime GitHub notifications bot (Oct 06 2023 at 18:37):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 19:00):alexcrichton submitted PR review:
Ok thanks! Let's resolve that question in parallel, and I think this is good to land.
Wasmtime GitHub notifications bot (Oct 06 2023 at 19:00):alexcrichton has enabled auto merge for PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 19:48):sunfishcode submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 19:48):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
shutdownto wake them up. We should be able to delete this both here and in the TCP impl now.
Wasmtime GitHub notifications bot (Oct 06 2023 at 23:01):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 06 2023 at 23:02):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2023 at 23:02):rvolosatovs created PR review comment:
I've removed it from both
Wasmtime GitHub notifications bot (Oct 07 2023 at 13:16):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 07 2023 at 18:32):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Oct 08 2023 at 15:26):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 09 2023 at 03:12):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Oct 09 2023 at 07:40):rvolosatovs updated PR #7148.
Wasmtime GitHub notifications bot (Oct 09 2023 at 13:40):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Oct 09 2023 at 14:29):alexcrichton merged PR #7148.
Last updated: Nov 22 2024 at 16:03 UTC