Replace the IoVec struct with IoSlice and IoSliceMut from the standard library by notgull · Pull Request #1643 · nix-rust/nix

notgull · 2022-01-23T20:05:31Z

As per discussion in #1637, the IoVec<&[u8]> and IoVec<&mut [u8]> types have been replaced with std::io::IoSlice and IoSliceMut, respectively. Notable changes made in this pull request include:

The complete replacement of IoVec with IoSlice* types in both public API, private API, and tests.
Replacing IoVec with IoSlice in docs.
Replacing &[IoVec<&mut [u8]>] with &mut [IoSliceMut], note that the slice requires a mutable reference now. This is how it's done in the standard library, and there might be a soundness issue in doing it the other way.

Resolves #1637

notgull · 2022-01-23T20:34:45Z

Checks appear to be failing because IoSlice does not implement Hash, PartialEq, and some other common traits. Not sure how it would be preferred to respond to this.

asomers

Don't forget a CHANGELOG, too!

asomers · 2022-01-23T21:25:53Z

-        use crate::sys::uio::IoVec;
+        use std::io::IoSlice;

        #[derive(Clone, Debug, Eq, Hash, PartialEq)]


This is causing failures, because IoSlice doesn't implement some of these traits. But SendfileHeaderTrailer isn't pub, so there's no penalty to remove some of its trait impls. What happens if you remove the offending trait impls? If it breaks something else, then you'll probably have to manually implement them, rather than derive them.

asomers · 2022-01-23T21:28:25Z

 /// and consisting of `len` bytes.
 ///
-/// This is the same underlying C structure as [`IoVec`](struct.IoVec.html),
+/// This is the same underlying C structure as `IoVec`,


Suggested change

/// This is the same underlying C structure as `IoVec`,

/// This is the same underlying C structure as `IoSlice`,

notgull · 2022-01-24T01:01:07Z

The CI errors appear to be due to a clippy error unrelated to this pull request.

asomers · 2022-01-24T01:07:39Z

The CI errors appear to be due to a clippy error unrelated to this pull request.

I don't think so. I think those errors were introduced by this PR. nmount uses IoVec/IoSlice.

notgull · 2022-01-24T02:28:13Z

Remainder of CI failures appear to be due to the aforementioned clippy error

asomers · 2022-01-24T02:36:21Z

The ptrace warning is fixed in master. You need to rebase.

notgull · 2022-01-27T02:05:48Z

Is there anything else that I need to do here before I can merge this request?

asomers

Sorry for the slow response, I've been working on other things lately. This looks 99% good; I just have a few comments about nmount.

asomers · 2022-03-06T15:19:23Z

@@ -371,19 +371,27 @@ impl<'a> Nmount<'a> {
    pub fn nmount(&mut self, flags: MntFlags) -> NmountResult {
        // nmount can return extra error information via a "errmsg" return


This comment got separated from the code it describes. It should be directly above line 380.

asomers · 2022-03-06T15:22:35Z

+
+        // N.B. notgull: the last entry here is technically an IoSliceMut.
+        //               this is a workaround to keep soundness in place
+        let mut iovecs: Vec<libc::iovec> =


Instead of doing a const transmutation, should self.iov be a vec of IoSliceMut instead?

I don't think so. nmount's methods by default takes several immutable pointers. Maybe we could just make it a list of libc::iovec by default?

I guess the problem is that the C API isn't const-correct. All of nmount's arguments are supposed to be const, except for the errmsg argument. So a transmutation like you had is probably Nix's best option.

asomers

I guess whether you do a mem::transmute or a pointer cast, it's equally ugly, and it's C's fault. This PR looks almost good to me, but I suggest getting rid of the #[inline] on private methods.

asomers · 2022-03-09T03:19:37Z

 #[cfg_attr(docsrs, doc(cfg(all())))]
 impl<'a> Nmount<'a> {
+    /// Helper function to push a slice onto the `iov` array.
+    #[inline]


There's no need to specify #[inline] for private methods.

asomers · 2022-03-24T02:58:57Z

Could you please rebase? You've got some conflicts now, and a rebase will also fix the test failure on uclibc.

asomers · 2022-03-25T22:53:22Z

@notgull are you sure you rebased to the current master? It doesn't look like it.

SUPERCILEX · 2022-04-06T07:21:19Z

Should this be blocking the 0.24 release? It'd be nice to have the feature splits out the door.

rtzoeller · 2022-04-07T23:18:48Z

@asomers will you have a chance to look at this in the next week or so? If not I can take a look; I agree with @SUPERCILEX that it'd be nice to get a 0.24.0 out soon (even if it means releasing a smaller 0.25.0 shortly thereafter).

asomers

It LGTM, apart from a few minor nits.

asomers · 2022-04-08T19:35:53Z

Ok, it looks like you've made all requested changes and the tests pass. Would you please squash now? Then we'll merge it.

notgull · 2022-04-08T20:00:33Z

Squashed and ready to rumble!

asomers

bors r+

SUPERCILEX · 2022-04-08T20:14:46Z

Thanks everyone for helping get 0.24 out! :)

bors · 2022-04-08T20:22:10Z

asomers requested changes Jan 23, 2022

View reviewed changes

irgstg mentioned this pull request Jan 27, 2022

IoVec::from_mut_slice and vmsplice are unsound #1647

Closed

notgull requested a review from asomers February 13, 2022 18:43

asomers reviewed Mar 6, 2022

View reviewed changes

asomers reviewed Mar 9, 2022

View reviewed changes

asomers added this to the 0.24.0 milestone Mar 13, 2022

asomers requested changes Apr 8, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread src/sys/socket/mod.rs Outdated

Comment thread src/sys/uio.rs Outdated

Replace the IoVec type with IoSlice and IoSliceMut

0b58f29

asomers approved these changes Apr 8, 2022

View reviewed changes

bors bot merged commit 256707e into nix-rust:master Apr 8, 2022

notgull deleted the ioslices branch April 8, 2022 20:22

sanmai-NL mentioned this pull request Feb 6, 2024

Make Submitter::register_buffers to accept IoSlice{Mut} tokio-rs/io-uring#259

Open

	/// This is the same underlying C structure as `IoVec`,
	/// This is the same underlying C structure as `IoSlice`,

		@@ -371,19 +371,27 @@ impl<'a> Nmount<'a> {
		pub fn nmount(&mut self, flags: MntFlags) -> NmountResult {
		// nmount can return extra error information via a "errmsg" return

Conversation

notgull commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notgull commented Jan 23, 2022

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

asomers Jan 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asomers Jan 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

notgull commented Jan 24, 2022

Uh oh!

asomers commented Jan 24, 2022

Uh oh!

notgull commented Jan 24, 2022

Uh oh!

asomers commented Jan 24, 2022

Uh oh!

notgull commented Jan 27, 2022

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

asomers Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

asomers Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

notgull Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

asomers Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

asomers Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

asomers commented Mar 24, 2022

Uh oh!

asomers commented Mar 25, 2022

Uh oh!

SUPERCILEX commented Apr 6, 2022

Uh oh!

rtzoeller commented Apr 7, 2022

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asomers commented Apr 8, 2022

Uh oh!

notgull commented Apr 8, 2022

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Apr 8, 2022

Uh oh!

bors bot commented Apr 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

notgull commented Jan 23, 2022 •

edited

Loading