-
Notifications
You must be signed in to change notification settings - Fork 177
Add write_stream support to Write opcode
#381
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
base: master
Are you sure you want to change the base?
Conversation
Expose the write_stream field from io_uring_sqe for IORING_OP_WRITE. Uses Option<u8> to only set the field when explicitly configured, avoiding potential issues on older kernels that don't support this feature.
Make write_stream support optional via cargo feature. Only compiles write_stream field and setter when feature is enabled, ensuring compatibility with older kernel versions.
Use cfg_attr to apply allow(dead_code) annotation only when write_stream feature is disabled. This is more targeted than module-level suppression.
eabcd3b to
8fadf0f
Compare
| ioprio: u16 = 0, | ||
| rw_flags: i32 = 0 | ||
| rw_flags: i32 = 0, | ||
| #[cfg_attr(not(feature = "write_stream"), allow(dead_code))] |
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 don't think this need a feature.
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'm more than happy to remove the feature!!
I want to call this out again; I personally think not supporting old kernels is fine, but I expect some people will be kind of unhappy:
A new
write_streamfeature is used to enable this capability. This allows kernels <6.16 to still compile, while newer kernel users can enable write_stream to get the feature.
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.
It should work correctly on older kernels as long as the user doesn't set the flag.
We don't use a feature for every opcode flag.
src/opcode.rs
Outdated
| rw_flags: i32 = 0 | ||
| rw_flags: i32 = 0, | ||
| #[cfg_attr(not(feature = "write_stream"), allow(dead_code))] | ||
| write_stream: Option<u8> = None |
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.
Is Option required? can it be u8?
src/opcode.rs
Outdated
| sqe.__bindgen_anon_3.rw_flags = rw_flags as _; | ||
| #[cfg(feature = "write_stream")] | ||
| if let Some(stream) = write_stream { | ||
| sqe.__bindgen_anon_5.__bindgen_anon_2.write_stream = stream; |
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.
It would be nice if we had a test.
Simplify the write_stream API by changing from Option<u8> to a plain u8 with default value 0. When write_stream is 0, it is not set on the SQE. This removes the need for conditional compilation in the struct destructuring and simplifies the code.
Add a test for the write_stream feature to io-uring-test. The test verifies that write_stream=1 works correctly by performing a write and read operation.
Resolves #380.
Expose the write_stream field from
io_uring_sqeforIORING_OP_WRITE. UsesOption<u8>to only set the field when explicitly configured.caveats
This PR has big caveats, on an otherwise very simple change:
write_streamfeature is used to enable this capability. This allows kernels <6.16 to still compile, while newer kernel users can enablewrite_streamto get the feature.write_streamfeature isn't on we got adead_codewarning, for code inside theopcode!macro. Added anallow(dead_code)there.