Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: send UDP datagrams #71

Merged
merged 1 commit into from
Dec 5, 2023
Merged

feat: send UDP datagrams #71

merged 1 commit into from
Dec 5, 2023

Conversation

mlegner
Copy link
Contributor

@mlegner mlegner commented Dec 1, 2023

Depends on #68

Closes #60

@mlegner mlegner added the rust label Dec 1, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 1, 2023 •
edited
Loading

Code Coverage

Package Line Rate Health
crates/scion-proto/src 65% ➖
crates/scion-proto/src/packet 51% ➖
crates/scion-proto/src/reliable 97% ✔
crates/scion/src 56% ➖
crates/scion-proto/src/path/metadata 100% ✔
crates/scion-proto/src/path 81% ✔
crates/scion/src/daemon 94% ✔
crates/scion-proto/src/address 69% ➖
crates/scion-proto/src/packet/headers 59% ➖
Summary 64% (712 / 1115) ➖

@mlegner mlegner force-pushed the feat/udp-sending branch 4 times, most recently from 0c58a25 to 5ba57e9 Compare December 4, 2023 09:25
@mlegner
Copy link
Contributor Author

mlegner commented Dec 4, 2023

@jpcsmith This PR still needs some cleanup work and tests.

However, if you have some time, I would be interested in your feedback on the general approach, in particular the mechanisms to prevent encoding/copying headers too many times.

Note that this includes the current version of #68.

@mlegner mlegner marked this pull request as ready for review December 4, 2023 19:39
@mlegner
Copy link
Contributor Author

mlegner commented Dec 4, 2023

@jpcsmith I've made most of the changes we discussed offline today. What I haven't touched yet is the setup of UdpSocket, UdpSocketInner, and State and their respective fields. I would leave those unchanged for now, we could then revisit that when we add the path-selection logic.

jpcsmith
jpcsmith previously approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@jpcsmith jpcsmith left a comment

Choose a reason for hiding this comment

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

Hey Markus, looks good, just a couple of ideas for removing the vector allocation when encoding and other refinements.

I still not sold on the extract() as I feel that the map implementation that you implemented already provides 90% of the benefit with an already accepted semantic in rust.

I suspect we may be missing an address type: ScionAddress or similar that is an IsdAsn + Host, as SocketAddr was used in places where a port should not be necessary. I will add an issue for this.

crates/scion-proto/src/datagram.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/datagram.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/datagram.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/wire_encoding.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/wire_encoding.rs Outdated Show resolved Hide resolved
crates/scion/src/udp_socket.rs Outdated Show resolved Hide resolved
crates/scion/src/udp_socket.rs Outdated Show resolved Hide resolved
crates/scion/src/udp_socket.rs Show resolved Hide resolved
crates/scion-proto/src/packet/headers.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/packet/headers/common_header.rs Outdated Show resolved Hide resolved
@jpcsmith jpcsmith self-requested a review December 5, 2023 11:46
@jpcsmith jpcsmith dismissed their stale review December 5, 2023 11:48

Did not intend to approve.

@mlegner
Copy link
Contributor Author

mlegner commented Dec 5, 2023

Thanks a lot for the detailed review, @jpcsmith.

I've removed the Extract trait in c5c130f.

I will address all other comments and re-request a review when I'm done.

@mlegner mlegner mentioned this pull request Dec 5, 2023
Open
@mlegner mlegner requested review from jpcsmith and removed request for jpcsmith December 5, 2023 15:31
@mlegner mlegner marked this pull request as draft December 5, 2023 15:33
@mlegner mlegner marked this pull request as ready for review December 5, 2023 16:47
@mlegner
Copy link
Contributor Author

mlegner commented Dec 5, 2023

@jpcsmith Again, thank you very much for the valuable suggestions. I've addressed most of them, there are just a couple of open questions from my side.

jpcsmith
jpcsmith approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@jpcsmith jpcsmith left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for your thorough implementation. I think my remaining comments can be delayed until the library grows a bit.

I think SendError enum can be simplified a bit with a few manual From<> implementations, but otherwise, this is good to go.

crates/scion-proto/src/packet/udp.rs Show resolved Hide resolved
crates/scion/src/udp_socket.rs Outdated Show resolved Hide resolved
crates/scion/src/udp_socket.rs Show resolved Hide resolved
This was referenced Dec 5, 2023
Closed
Open
@mlegner
feat: UDP sending and various refactorings
3e5a811 
- move datagram to scion-proto
- reorganize modules, in particular in `scion_proto::packet`
- distinguish between `ScionPacketRaw` and `ScionPacketUdp`
- more consistent encoding implementations with the `WireEncode` and
  `WireEncodeVec` traits
@mlegner mlegner enabled auto-merge (rebase) December 5, 2023 18:06
@mlegner mlegner merged commit 09f2142 into main Dec 5, 2023
11 checks passed
@mlegner mlegner deleted the feat/udp-sending branch December 5, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpcsmith jpcsmith jpcsmith approved these changes

Assignees
No one assigned
Labels
rust
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement UDP datagram sending.
2 participants
@mlegner @jpcsmith