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.

Sign up for GitHub

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

Jump to bottom

Expose preadv and pwritev on BSDs #883

Merged
merged 1 commit into from Apr 19, 2018
Merged

Expose preadv and pwritev on BSDs #883

merged 1 commit into from Apr 19, 2018

Conversation

morrowa
Copy link
Contributor

@morrowa morrowa commented Apr 13, 2018

This addresses issue #690.

It exposes the preadv and pwritev functions on supported BSDs (not including Apple platforms).

@morrowa morrowa mentioned this pull request Apr 13, 2018
Closed
Susurrus
Susurrus suggested changes Apr 13, 2018
View reviewed changes
Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add some docs above the [#cfg...] line for both of these functions since they're missing them?

src/sys/uio.rs Outdated
@@ -19,7 +19,11 @@ pub fn readv(fd: RawFd, iov: &mut [IoVec<&mut [u8]>]) -> Result<usize> {
Errno::result(res).map(|r| r as usize)
}

#[cfg(target_os = "linux")]
#[cfg(any(target_os = "linux",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please alphabetize the list of target OSes.

@morrowa
Copy link
Contributor Author

morrowa commented Apr 15, 2018

@Susurrus ready for re-review!

Susurrus
Susurrus suggested changes Apr 15, 2018
View reviewed changes
Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments apply to the other function as well.

Also, CI is failing, so please look into that.

src/sys/uio.rs
@@ -19,7 +19,16 @@ pub fn readv(fd: RawFd, iov: &mut [IoVec<&mut [u8]>]) -> Result<usize> {
Errno::result(res).map(|r| r as usize)
}

#[cfg(target_os = "linux")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comments should start with a short summary line, like git commit messages. Then a blank line and a longer descriptive message afterwards. See the Rust book.

src/sys/uio.rs Outdated
/// until all buffers have been written or an error occurs. The file offset is
/// not changed.
///
/// See also: `writev` and `pwrite`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please link to these functions using inline links.

@morrowa
Copy link
Contributor Author

morrowa commented Apr 15, 2018

I don't understand what you mean that "the comments apply to the other function as well." How do I fix that?

@Susurrus
Copy link
Contributor

My review comments apply to both of the functions you've added.

@morrowa
Copy link
Contributor Author

morrowa commented Apr 16, 2018

How does the documentation look?

When I checked on the failed builds before my push, they didn't show any steps or errors. Now those two aren't showing up at all. Is the build machine/service having trouble?

@asomers
Copy link
Member

asomers commented Apr 16, 2018

Looks like the build machine hit ENOSPC :(. I'll fix it.

@asomers asomers self-requested a review April 16, 2018 14:33
asomers
asomers approved these changes Apr 16, 2018
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all looks fine to me now.

@Susurrus
Copy link
Contributor

Awesome, looks great, thanks!

bors r+

bors bot added a commit that referenced this pull request Apr 18, 2018
@bors @morrowa
Merge #883
84ef78f 
883: Expose `preadv` and `pwritev` on BSDs r=Susurrus a=morrowa

This addresses issue #690.

It exposes the `preadv` and `pwritev` functions on supported BSDs (not including Apple platforms).

Co-authored-by: Andrew Morrow <andrew.d.morrow@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 18, 2018

Timed out

@morrowa
Copy link
Contributor Author

morrowa commented Apr 18, 2018

I don't have permission to follow Bors' details link. Is there anything I can correct?

@asomers
Copy link
Member

asomers commented Apr 19, 2018

No. I need to rebuild the i386 builder.

@asomers
Copy link
Member

asomers commented Apr 19, 2018

Er, I forgot that I had already done that 2 days ago. But Buildbot was stuck and needed a restart, which I have done. Let's try again.

bors retry

@asomers
Copy link
Member

asomers commented Apr 19, 2018

Actually, on second thought, could you please squash the commits?

bors r-

@morrowa
Copy link
Contributor Author

morrowa commented Apr 19, 2018

Squashed!

@asomers
Copy link
Member

asomers commented Apr 19, 2018

bors r+

bors bot added a commit that referenced this pull request Apr 19, 2018
@bors @morrowa
Merge #883
305141c 
883: Expose `preadv` and `pwritev` on BSDs r=asomers a=morrowa

This addresses issue #690.

It exposes the `preadv` and `pwritev` functions on supported BSDs (not including Apple platforms).

Co-authored-by: Andrew Morrow <andrew.d.morrow@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 19, 2018

Build succeeded

@bors bors bot merged commit 3502b03 into nix-rust:master Apr 19, 2018
@morrowa morrowa deleted the 690_preadv_pwritev_platforms branch April 26, 2018 02:53
@hidva
Copy link

hidva commented Jan 17, 2021

Hi, Please tell me why pwritev can not used in Apple platforms?

Thanks

@morrowa
Copy link
Contributor Author

morrowa commented Jan 17, 2021

These two functions do not exist in any version of macOS 10. It looks like they have been added in macOS 11 per this external patch:

https://mail.gnu.org/archive/html/qemu-devel/2020-10/msg03484.html

@hidva
Copy link

hidva commented Jan 18, 2021

These two functions do not exist in any version of macOS 10. It looks like they have been added in macOS 11 per this external patch:

https://mail.gnu.org/archive/html/qemu-devel/2020-10/msg03484.html

Yes, so is there a plan to add preadv / pwritev to nix? Currently, I have to implement as follow:

#[cfg(target_os = "linux")]
fn pwritev(fd: RawFd, iov: &[IoVec<&[u8]>], offset: off_t) -> nix::Result<usize> {
    use nix::sys::uio::pwritev as _pwritev;
    _pwritev(fd, iov, offset)
}

#[cfg(target_os = "macos")]
fn pwritev(fd: RawFd, iov: &[IoVec<&[u8]>], offset: off_t) -> nix::Result<usize> {
    use nix::sys::uio::pwrite;
    let mut buff = Vec::<u8>::new();
    for iv in iov {
        buff.extend_from_slice(iv.as_slice());
    }
    pwrite(fd, buff.as_slice(), offset)
}

https://github.com/hidva/KuiBaDB/blob/master/src/bin/kuiba/access/wal.rs#L24

Thanks.

@morrowa
Copy link
Contributor Author

morrowa commented Jan 25, 2021

I am not upgrading to macOS 11 yet because too many of my peripherals don't yet support it. You are welcome to implement the change yourself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

@Susurrus Susurrus Susurrus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@morrowa @Susurrus @asomers @hidva