types: add RecvMsgOut helper structure #192
Conversation
There was a problem hiding this comment.
That test is very nice. If you're sure you don't need to submit and wait for 3, it's fine with me.
Well, I do prefer a new opcode were used to convey this different operation to the user as a few things need to all be done correctly but it does work as you've shown.
src/types.rs
Outdated
| return Err(()); | ||
| } | ||
|
|
||
| let header = sys::io_uring_recvmsg_out { |
There was a problem hiding this comment.
I think it might be better to use read_unaligned because the size of io_uring_recvmsg_out may change in future. I can imagine it might use u16 on some platforms.
src/types.rs
Outdated
| /// `buffer` is the whole buffer previously provided to the ring, while `name_len` | ||
| /// and `control_len` are equal to the input lengths provided through `msghdr` in | ||
| /// the corresponding SQE. | ||
| pub fn parse(buf: &'buf [u8], name_len: u32, control_len: u32) -> Result<Self, ()> { |
There was a problem hiding this comment.
I perfer to use msghdr as a parameter instead of its field (like liburing), to avoid user wrong input.
src/types.rs
Outdated
| let name_start = Self::DATA_START; | ||
| let name_size = u32::min(self.header.namelen, self.msghdr_name_len); | ||
| let name_end = name_start + name_size as usize; | ||
| &self.buf[name_start..name_end] |
There was a problem hiding this comment.
maybe it would be better to save a reference while parse? splitting it into different methods might make compiler hard to optimize bounds check.
There was a problem hiding this comment.
Could you add some more details about what you are suggesting here?
My feeling is that what you want to do goes against the limit of having self-references within RecvMsgOut (the buffer, plus the subfields pointing within it), but maybe I misunderstood what you have in mind.
There was a problem hiding this comment.
On the topic of performances, I did tweak the logic a bit so that all accessor methods can use unchecked/saturating math.
There was a problem hiding this comment.
It is an immutable reference, there is no self-referencing problem. We also don't need to keep original buf, we can use split_at to split.
There was a problem hiding this comment.
Ack, thanks, initially I didn't understand what you were suggesting. I've changed the parsing logic so that the buffer is split upfront and the structure only keeps references to the three specific data slices.
src/types.rs
Outdated
|
|
||
| /// Message control data, with the same semantics as `msghdr.msg_control`. | ||
| pub fn control_data(&self) -> &[u8] { | ||
| let control_start = Self::DATA_START + self.msghdr_name_len as usize; |
There was a problem hiding this comment.
398L maybe take header.namelen, but use msghdr_name_len unconditionally here, which is somewhat inconsistent
There was a problem hiding this comment.
This should be correct as is, because the fixed length of the name field is msghdr_name_len, while the dynamic length of the name data is the minimum between the field length and the length of the actual result data. In this case we want to skip the whole field, whose length is fixed upfront regardless of the result.
But I agree this may be confusing. I've added a docstring on both msghdr_name_len and msghdr_control_len which hopefully could clarify the semantics.
lucab
force-pushed
the
ups/recvmsg-multishot
branch
2 times, most recently
from
84e7323
to
2be3eeb
Compare
| let header: sys::io_uring_recvmsg_out = | ||
| unsafe { std::ptr::read_unaligned(buffer.as_ptr() as _) }; | ||
|
|
||
| let msghdr_name_len = msghdr.msg_namelen as _; |
There was a problem hiding this comment.
It's better to use .into() whenever possible
There was a problem hiding this comment.
This is a u32 -> usize conversion, which is always safe to perform on Linux but unfortunately not yet covered in Rust by Into (see rust-lang/rust#70460).
Additionally there are some mismatches across different libc, so that the source fields are u32 on some libc and usize on other libc (so actually not really casting). This is all very unfortunate, but as _ is pretty much a required hack to appease rustc and clippy in a portable way across libc's.
This introduces a `RecvMsgOut` structure which allows consumers to safely parse the buffered result from a multishot `IORING_OP_RECVMSG` completion event.
This adds a test entry which exercises `IORING_RECV_MULTISHOT` on `IORING_OP_RECVMSG`, as well as `RecvMsgOut` parsing logic.
lucab
force-pushed
the
ups/recvmsg-multishot
branch
from
2be3eeb
to
65ce2c5
Compare
|
Thanks everyone! |
This introduces a
RecvMsgOutstructure which allows consumers tosafely parse the buffered result from a multishot
IORING_OP_RECVMSGcompletion event.