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 trait to reduce duplicated code between socket address types, implement netlink addresses #1004

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kevinmehall
Copy link
Contributor

@kevinmehall kevinmehall commented Jan 29, 2024

#918

This adds a trait SockAddr, and implements it for SocketAddrV4, SocketAddrV6, SocketAddrUnix, SocketAddrXdp, SocketAddr, SocketAddrAny, and the newly-added SocketAddrNetlink. The syscalls that take a sockaddr are reimplemented in terms of this trait, removing hundreds of lines of duplicated code. It also simplifies the public interface, allowing rustix::net::{bind, connect, sendto, sendmsg_addr} to take an (addr: &impl SockAddr), rather than needing separate functions bind_v4, bind_v6, bind_unix, bind_xdp, bind_any etc. All of those are left as wrappers around the generic version, but could be deprecated and removed.

Open questions

  • SocketAddrAny: So far this only changes the syscalls that take a socket address as an in-pointer. Those that return an address via an out-pointer currently return SocketAddrAny. If it is desirable to allow types outside of rustix to implement the SocketAddress trait, it would be ideal for SocketAddrAny to wrap sockaddr_storage instead of being a Rust enum. This would allow SocketAddrAny be fully generic and allow these syscalls to be used with any address type not known to rustix. Address types could use .into() / .try_into() to convert to / from SocketAddrAny. This would require a breaking change.

@sunfishcode
Copy link
Member

As a quick update here, I am looking forward to reviewing this; I've just been busy.

Copy link

@niklaswimmer niklaswimmer left a comment

Choose a reason for hiding this comment

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

I like this approach, it allows me to very easily define a socket address not yet present in this crate, like so

#[derive(Debug, Clone, Copy)]
#[allow(dead_code)] // fields are never read directly because they get memcp'd into another struct
#[repr(C)]
struct SocketAddressHci {
    hci_family: rustix::net::AddressFamily,
    hci_dev: u16,
    hci_channel: u16,
}

unsafe impl SocketAddress for SocketAddressHci {
    /// SAFETY: `SocketAddressHci` has the same layout as the type in the kernel and can therefore safely be passed in a syscall
    type CSockAddr = Self;

    fn encode(&self) -> Self::CSockAddr {
        self.clone()
    }
}

I left some comment about the code structure, most of it more nit than anything else. I am interested to here your opinion on using something like a Bow for the return value of encode tho. It would allow impl's to avoid the clone without having to reimplement write_sockaddr and with_sockaddr again :)

src/backend/linux_raw/net/mod.rs Outdated Show resolved Hide resolved
src/backend/linux_raw/net/addr.rs Outdated Show resolved Hide resolved
src/backend/linux_raw/net/addr.rs Outdated Show resolved Hide resolved
src/backend/linux_raw/net/addr.rs Outdated Show resolved Hide resolved
src/backend/libc/net/msghdr.rs Outdated Show resolved Hide resolved
src/net/socket_address.rs Outdated Show resolved Hide resolved
src/net/socket_address.rs Outdated Show resolved Hide resolved
@kevinmehall
Copy link
Contributor Author

Given that sockaddr was included on the list of libc types to remove in #1063, I slimmed down the SockAddr trait further to only with_sockaddr and made the raw pointer type an opaque struct to avoid all libc types in the API.

I also rebased to clean up the messy history for easier review. Full history archived here.

@kevinmehall kevinmehall marked this pull request as ready for review June 1, 2024 23:34
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.

3 participants