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 sendmmsg(2) on linux #1171

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

colinmarc
Copy link
Contributor

https://man7.org/linux/man-pages/man2/sendmmsg.2.html

Partially addresses #1156. I would've liked to add recvmmsg in the same PR, but it's actually much more complicated.

@colinmarc colinmarc force-pushed the sendmmsg branch 3 times, most recently from 10452dc to 779b2b7 Compare September 18, 2024 21:34
@colinmarc

This comment was marked as resolved.

@colinmarc colinmarc force-pushed the sendmmsg branch 2 times, most recently from e6b2394 to 6534026 Compare September 19, 2024 07:42
#[cfg(target_os = "linux")]
impl<'a> MMsgHdr<'a> {
/// Constructs a new message with no destination address.
pub fn new(iov: &[IoSlice<'a>], control: &mut SendAncillaryBuffer<'_, '_, '_>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment on why control needs to be a mut reference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dumb answer is "because sendmsg requires a mut reference" (as does with_noaddr_msghdr and friends). I assumed some random API modified the cmsg on send, but I don't know which one.

tests/net/v4.rs Show resolved Hide resolved
src/net/send_recv/msg.rs Outdated Show resolved Hide resolved
src/net/send_recv/msg.rs Outdated Show resolved Hide resolved
@colinmarc
Copy link
Contributor Author

I think I addressed all your comments.

src/net/send_recv/msg.rs Outdated Show resolved Hide resolved
src/net/send_recv/msg.rs Outdated Show resolved Hide resolved
@colinmarc colinmarc force-pushed the sendmmsg branch 4 times, most recently from 695e084 to e960980 Compare October 18, 2024 17:06
@colinmarc colinmarc requested a review from notgull December 2, 2024 09:23
@colinmarc
Copy link
Contributor Author

Hi, anything needed from me on this?

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

I think this looks good, just a few more review comments:

src/net/socket_addr_any.rs Outdated Show resolved Hide resolved
src/net/send_recv/msg.rs Outdated Show resolved Hide resolved
src/backend/linux_raw/net/syscalls.rs Outdated Show resolved Hide resolved
src/net/send_recv/msg.rs Outdated Show resolved Hide resolved
@colinmarc
Copy link
Contributor Author

colinmarc commented Feb 10, 2025

Sorry for pinging and then disappearing - was really sick :)

I rebased and addressed your comments. Please let me know how you'd like to resolve the overlap with #1004. It seems like I could use the updated SockAddrAny from that instead of RawSocketAddr.

@colinmarc
Copy link
Contributor Author

Rebased on top of #1004 now that it's in main. The test failures look unrelated.

This patch is a lot smaller now!


/// Constructs a new message to a specific address.
pub fn new_with_addr(
addr: &'a SocketAddrAny,
Copy link
Contributor Author

@colinmarc colinmarc Feb 11, 2025

Choose a reason for hiding this comment

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

I would like to take a &'a impl SockAddrArg here, but it doesn't seem like the trait ensures that the pointer passed to the closure lives as long as the SockAddrArg itself.

We could obviously immediately call as_any, but that would just be making an explicit clone implicit.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right. Could you add a comment about this? Something like, "this doesn't use SocketAddrArg because that creates a temporary SocketAddrAny that only lives for the duration of the with_sockaddr call, and we need a SocketAddrAny that lives for 'a".

Another possibility would be to add a with_socket_addr_any function to SocketAddrArg like this:

    fn with_socket_addr_any<R>(&self, f: impl FnOnce(&SocketAddrAny) -> R) -> R {
        let any = self.as_any();
        f(&any)
    }

Then users with a family-specific addr could do

    addr.with_socket_addr_any(|addr| {
        sendmmsg(
            &socket,
            &mut [
                MMsgHdr::new_with_addr(
                    &addr,
                    &[IoSlice::new(b"hello")],
                    &mut Default::default(),
                ),
            ],
            SendFlags::empty(),
        )
    });

and then SocketAddrAny's impl SocketAddrArg could skip the as_any() call:

    fn with_socket_addr_any<R>(&self, f: impl FnOnce(&SocketAddrAny) -> R) -> R {
        f(self)
    }

That way users could avoid calling as_any themselves. We could do that, though it's not that much of a simplification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a sentence to the docstring pointing users to as_any - let me know what you think or if you think it needs more explication. I tried a few variants and in the end it felt like too much detail for end-users of the library, but maybe it would be useful as an "internal" comment.

let sent = sendmmsg(
&data_socket,
&mut [
MMsgHdr::new(&[IoSlice::new(b"hello")], &mut Default::default()),
Copy link
Member

Choose a reason for hiding this comment

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

Should this use new_wiith_addr, like the v6 version does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my idea was to exercise both code paths. I changed it so that both tests exercise both.


/// Constructs a new message to a specific address.
pub fn new_with_addr(
addr: &'a SocketAddrAny,
Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right. Could you add a comment about this? Something like, "this doesn't use SocketAddrArg because that creates a temporary SocketAddrAny that only lives for the duration of the with_sockaddr call, and we need a SocketAddrAny that lives for 'a".

Another possibility would be to add a with_socket_addr_any function to SocketAddrArg like this:

    fn with_socket_addr_any<R>(&self, f: impl FnOnce(&SocketAddrAny) -> R) -> R {
        let any = self.as_any();
        f(&any)
    }

Then users with a family-specific addr could do

    addr.with_socket_addr_any(|addr| {
        sendmmsg(
            &socket,
            &mut [
                MMsgHdr::new_with_addr(
                    &addr,
                    &[IoSlice::new(b"hello")],
                    &mut Default::default(),
                ),
            ],
            SendFlags::empty(),
        )
    });

and then SocketAddrAny's impl SocketAddrArg could skip the as_any() call:

    fn with_socket_addr_any<R>(&self, f: impl FnOnce(&SocketAddrAny) -> R) -> R {
        f(self)
    }

That way users could avoid calling as_any themselves. We could do that, though it's not that much of a simplification.

@sunfishcode
Copy link
Member

The FreeBSD CI failure is fixed on main in #1322.

@sunfishcode sunfishcode merged commit 871dee3 into bytecodealliance:main Feb 11, 2025
45 checks passed
@sunfishcode
Copy link
Member

Thanks!

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