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.

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

Add support for std::net::{*Addr*} #30

Merged
merged 10 commits into from
Sep 12, 2024
Merged

Add support for std::net::{*Addr*} #30

merged 10 commits into from
Sep 12, 2024

Conversation

finnbear
Copy link
Member

@finnbear finnbear commented Aug 4, 2024

Works by converting them to/from existing implementations like Result<Ipv4Addr, Ipv6Addr>, u32, u128, and (Ipv4Addr, u16). This is to avoid additional unsafe code. Manual re-implementation might yield better performance.

@finnbear finnbear added the enhancement New feature or request label Aug 4, 2024
@finnbear finnbear mentioned this pull request Aug 4, 2024
14 tasks
@finnbear finnbear changed the title Add support for core::net::{IpAddr, Ipv4Addr, Ipv6Addr} Add support for std::net::{IpAddr, Ipv4Addr, Ipv6Addr} Aug 4, 2024
@inodentry
Copy link

What about SocketAddr? It's basically ip address + port, I imagine it shouldn't be hard to support. It's very common in network protocol messages.

@finnbear
Copy link
Member Author

finnbear commented Aug 12, 2024

What about SocketAddr?

Hmm; SocketAddr is tricker because we would have to decide how to handle SocketAddrV6's flowinfo and scope_id. impl serde::Serialize for SocketAddrV6 omits them and impl serde::Deserialize for SocketAddrV6 zero-initializes them. This leads to better JSON, for example, but could lead to data loss. Perhaps bitcode, which doesn't have a text format, should keep all the fields?

Edit:

poll:
🎉 ip + port + flowinfo + scope_id
😕 ip + port + flowinfo
🚀 ip + port

@finnbear finnbear changed the title Add support for std::net::{IpAddr, Ipv4Addr, Ipv6Addr} Add support for std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV4} Aug 13, 2024
@inodentry
Copy link

inodentry commented Aug 14, 2024

I agree with your logic that a binary non-human-readable format does not need to worry about being "pretty" and can use that to its advantage to be more complete/correct.

Further, encoding the extra fields is strictly more useful than not encoding them.

If I want to just store an IP address + port, I can just make them two separate fields in my struct (IpAddr + u16) and then construct a SocketAddr from that. It is therefore possible to "opt out" of encoding them, if the size of the encoded message is more important than preserving those fields, or if I don't care about them.

However, if the encoding omits them, now I have no way of preserving them, if I happen to have a use case where they are actually important. There is no way to "opt in".

Therefore, my opinion is that they should be encoded.

@finnbear finnbear changed the title Add support for std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV4} Add support for std::net::{*Addr*} Aug 14, 2024
@finnbear
Copy link
Member Author

finnbear commented Aug 14, 2024

Thank you, I personally agree! (this PR is now awaiting @caibear's review, although I already use it via a git dependency)

@caibear caibear merged commit 15fe3aa into main Sep 12, 2024
1 check passed
@caibear caibear deleted the ip_addr branch September 16, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants