-
Notifications
You must be signed in to change notification settings - Fork 672
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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:
-
Have a test for
sendmsg_pre_alloc()
, you can use the test here, and do some assertions after thesendmsg_pre_alloc()
call. We should add this test intest/sys/test_socket.rs
. -
Document that one can use
sendmsg_pre_alloc()
in the doc ofsendmsg()
:-/// Allocates if cmsgs is nonempty. +/// Allocates if cmsgs is nonempty, use [`sendmsg_pre_alloc()`] if you want to use a pre-allocated buffer.
-
Add a changelog entry for this PR, please take a look at CONTRIBUTING.md on how to do this.
src/sys/socket/mod.rs
Outdated
/// 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. |
There was a problem hiding this comment.
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:
/// 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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:
Line 2044 in 7d75bbc
(*p).msg_controllen = capacity as _; |
There was a problem hiding this comment.
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()
?
There was a problem hiding this comment.
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.
For the CI failure, you can allow the lint if necessary:
|
d7696eb
to
7aee29b
Compare
7aee29b
to
1eed9f4
Compare
closes #2457