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

Make Submitter::register_buffers to accept IoSlice{Mut} #259

Open
sanmai-NL opened this issue Feb 6, 2024 · 4 comments
Open

Make Submitter::register_buffers to accept IoSlice{Mut} #259

sanmai-NL opened this issue Feb 6, 2024 · 4 comments

Comments

@sanmai-NL
Copy link

sanmai-NL commented Feb 6, 2024

IoSlice and IoSliceMut are in the standard library. See: nix-rust/nix#1643 for some background.

io-uring's Submitter::register_buffers uses a libc::iovec.

I propose to adopt the standard library type in order to increase API compatibility.

@quininer
Copy link
Member

Welcome PR.

@connortsui20
Copy link
Contributor

I'm willing to make this PR and do some testing.

But which of IoSlice and IoSliceMut should replace libc::iovec? Or should both be supported? The more obvious answer to me is only accepting IoSliceMut since if we want to share these buffers with the kernel we probably can't easily make the guarantee that there are no shared references to the underlying slice. On the other hand, this is marked unsafe so it could be okay?

@connortsui20
Copy link
Contributor

All that needs to change is this if we only accept IoSliceMut:

pub unsafe fn register_buffers(&self, bufs: &[libc::iovec]) -> io::Result<()> {
// into
pub unsafe fn register_buffers(&self, bufs: &[IoSliceMut<'_>]) -> io::Result<()> {

Simply doing that and converting the iovec into IoSliceMut in the test_tcp_zero_copy_send_fixed test (plus the test that I wrote in #278) passes. Though to be clear those are the only tests that test register_buffers.

@LtdJorge
Copy link

opcode::Readv and opcode::Writev also depend on libc::iovec. I guess an alternative could be to just export libc::iovec.

penberg added a commit to tursodatabase/limbo that referenced this issue Jan 14, 2025
This shaves off `nix` as a dependency in core, which was only being used
in the io_uring backend for the `fcntl` advisory record locking. Since a
previous PR made `rustix` a dep not only for Mac but for any Unix, and
`rustix` also has `fcntl`, `nix` becomes redundant.
Furthermore, it reduces `libc` usage across core. The only remaining
uses are:
```rust
io_uring::opcode::Readv::new(fd, iovec as *const iovec as *const libc::iovec, 1)
io_uring::opcode::Writev::new(fd, iovec as *const iovec as *const libc::iovec, 1)
```
These two are a little ugly, but sadly the `io_uring` crate requires
both opcodes to take a `libc::iovec` while it doesn't export the type.
See tokio-rs/io-uring#259 for a request to use `std::io::IoSlice` or to
export the type directly.
Apart from those two, there are no other usages of libc, so if those are
resolved, we could also drop the dependency on libc.

Closes #668
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

No branches or pull requests

4 participants