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

Improve ergonomics between SockaddrIn and underlying libc type #2328

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

pronebird
Copy link
Contributor

@pronebird pronebird commented Mar 6, 2024

What does this PR do

Checklist:

  • I have read CONTRIBUTING.md

  • I have written necessary tests and rustdoc comments

  • A change log has been added if this PR modifies nix's API

  • Add Into<libc::sockaddr_in6> for SockaddrIn6

  • Add Into<libc::sockaddr_in> for SockaddrIn

My typical use case for these Into traits is when I am bridging from safe world into the world of pain (C). For example, presently I have to go over as_ref() to access the underlying sockaddr_inX . This is a bit verbose for my liking, so I figured it would be great to have into() to do the job for me. Usually From is preferred but I am not certain if we want that, hence I have implemented Into but can easily flip to From if that makes sense.

let addr_sa = SockaddrIn6::from(SocketAddrV6::new(addr, 0, 0, 0));
let mut req: in6_aliasreq = unsafe { mem::zeroed() };
-req.addr = *addr_sa.as_ref();
+req.addr = addr_sa.into();

I also noticed that repr(transparent) is set on these structs and naturally I thought I could use addr_sa as sockaddr_in6 but that doesn't seem to compile.

@pronebird pronebird force-pushed the sockaddr-into branch 3 times, most recently from 2c74fa0 to 1df9fb5 Compare March 6, 2024 12:51
@@ -920,6 +920,15 @@ impl From<SockaddrIn> for net::SocketAddrV4 {
}

#[cfg(feature = "net")]
#[allow(clippy::from_over_into)]
Copy link
Member

Choose a reason for hiding this comment

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

Would you like to do:

impl From<libc::sockaddr_in> for SockaddrIn
impl From<SockaddrIn> for libc::sockaddrin

impl From<libc::sockaddr_in6> for SockaddrIn6
impl From<SockaddrIn6> for libc::sockaddrin6

So that we can do roundtrips and also make clippy happy:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@SteveLauC
Copy link
Member

This is indeed something we should do to allow Nix to be used in such scenarios, see #2327, and thanks for your interest in contributing to Nix!

@pronebird
Copy link
Contributor Author

CI failure in test_sigsuspend is probably unrelated.

@pronebird pronebird requested a review from SteveLauC March 7, 2024 09:51
Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thanks! That CI failure is not related:)

@SteveLauC SteveLauC added this pull request to the merge queue Mar 7, 2024
Merged via the queue into nix-rust:master with commit 99f2dc2 Mar 7, 2024
34 of 35 checks passed
@pronebird pronebird deleted the sockaddr-into branch March 7, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants