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 preallocated buffer version of sendmsg #2498

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

howjmay
Copy link
Contributor

@howjmay howjmay commented Sep 12, 2024

closes #2457

@SteveLauC SteveLauC self-requested a review September 13, 2024 09:47
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.

Hi, thanks for the patch!

Besides that 2 comments, I hope we can also:

  1. Have a test for sendmsg_pre_alloc(), you can use the test here, and do some assertions after the sendmsg_pre_alloc() call. We should add this test in test/sys/test_socket.rs.

  2. Document that one can use sendmsg_pre_alloc() in the doc of sendmsg():

    -/// Allocates if cmsgs is nonempty.
    +/// Allocates if cmsgs is nonempty, use [`sendmsg_pre_alloc()`] if you want to use a pre-allocated buffer.
  3. Add a changelog entry for this PR, please take a look at CONTRIBUTING.md on how to do this.

Comment on lines 1644 to 1648
/// Send data in scatter-gather vectors to a socket, possibly accompanied
/// by ancillary data. Optionally direct the message at the given address,
/// as with sendto.
/// sendmsg_pre_alloc is the same as sendmsg but it accepts a preallocated
/// cmsg buffer vector.
Copy link
Member

Choose a reason for hiding this comment

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

Let's emphasize that pre_alloc part and link the original API:

Suggested change
/// Send data in scatter-gather vectors to a socket, possibly accompanied
/// by ancillary data. Optionally direct the message at the given address,
/// as with sendto.
/// sendmsg_pre_alloc is the same as sendmsg but it accepts a preallocated
/// cmsg buffer vector.
/// `sendmsg_pre_alloc()` is the same as [`sendmsg()`] but it accepts a preallocated
/// `cmsg` buffer vector.
///
/// Send data in scatter-gather vectors to a socket, possibly accompanied
/// by ancillary data. Optionally direct the message at the given address,
/// as with sendto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw can I rename the new API as sendmsg_prealloc(), I feel the underscore there is a little bit of redundant

Copy link
Member

Choose a reason for hiding this comment

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

btw can I rename the new API as sendmsg_prealloc()

Sure

flags: MsgFlags, addr: Option<&S>, cmsg_buffer: &mut Vec<u8>) -> Result<usize>
where S: SockaddrLike
{
let mhdr = pack_mhdr_to_send(&mut cmsg_buffer[..], iov, cmsgs, addr);
Copy link
Member

Choose a reason for hiding this comment

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

We need to ensure that cmsg_buffer has enough length to accommodate cmsgs before passing it to pack_mhdr_to_send()

Copy link
Member

Choose a reason for hiding this comment

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

The current impl of pack_mhdr_to_send() requires that cmsg_buffer.len() equals the length needed by control messages, which may not be true with the pre-alloc API, we need to change this:

(*p).msg_controllen = capacity as _;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can add a if-condition in sendmsg_pre_alloc() which will panic if cmsg_buffer.len() != cmsgs .len()?

Copy link
Member

Choose a reason for hiding this comment

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

I kinda think that would be too limited? There should be no issue if cmsg_buffer.len()> cmsg.len(), my preferred way is to: check if cmsg_buffer.len() is greater than or equal to cmsg.len(), if not, return Err(Errno::ENOBUFS). For the pack_mhdr_to_send() function, we can add another cmsg_len argument, and document that cmsg_buffer.len() could be greater than cmsg_len due to the this sendmsg_pralloc() function.

@SteveLauC
Copy link
Member

For the CI failure, you can allow the lint if necessary:

error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do
    --> src/sys/socket/mod.rs:1650:64
     |
1650 |                flags: MsgFlags, addr: Option<&S>, cmsg_buffer: &mut Vec<u8>) -> Result<usize>
     |                                                                ^^^^^^^^^^^^ help: change this to: `&mut [u8]`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg
     = note: `-D clippy::ptr-arg` implied by `-D warnings`

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.

Allow sendmsg to accept a preallocated Vec
2 participants