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

sys::sendfile adding solaris' sendfilev wrapper proposal. #2207

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

devnexen
Copy link
Contributor

No description provided.

src/sys/sendfile.rs Outdated Show resolved Hide resolved
src/sys/sendfile.rs Outdated Show resolved Hide resolved
@devnexen devnexen force-pushed the sendfilev_solaris branch 4 times, most recently from 593c1d4 to 6f87e54 Compare November 25, 2023 09:51
@devnexen devnexen marked this pull request as ready for review November 25, 2023 09:51
src/sys/sendfile.rs Outdated Show resolved Hide resolved
src/sys/sendfile.rs Outdated Show resolved Hide resolved
src/sys/sendfile.rs Outdated Show resolved Hide resolved
src/sys/sendfile.rs Outdated Show resolved Hide resolved
src/sys/sendfile.rs Outdated Show resolved Hide resolved
src/sys/sendfile.rs Outdated Show resolved Hide resolved
test/test_sendfile.rs Outdated Show resolved Hide resolved
test/test_sendfile.rs Outdated Show resolved Hide resolved

/// initialises SendfileVec to send data from `fd`.
pub fn new<F: AsFd>(
fd: F,
Copy link
Member

Choose a reason for hiding this comment

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

This still doesn't capture anything about fd's lifetime. With this API, it's possible to use a RawFd after the file has been closed. That's why I suggested using BorrowedFd. You might be able to do it, without additional runtime cost, like this:

pub struct SendfileVec<'a> {
    raw: libc::sendfilevec_t,
    phantom: PhantomData<BorrowedFd<'a>>
}
pub fn new<F: AsFd, 'a>(fd: &'a F, off: off_t, len: usize) -> Self<'a> {
    let bfd = fd.as_fd();
    Self{raw: libc::sendfilevec_t{sfv_fd: bfd.as_raw_fd(), ...}, phantom: PhantomData}}
}

Copy link
Member

Choose a reason for hiding this comment

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

We should probably use BorrowedFd<'a> here so that it is consistent with FdSet and PollFd

BTW, I think we should also do this to Epoll

src/sys/sendfile.rs Outdated Show resolved Hide resolved
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.

Nice! It looks a lot more efficient now.

phantom: PhantomData<BorrowedFd<'a>>
}

const SFV_FD_SELF: i32 = -2;
Copy link
Member

Choose a reason for hiding this comment

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

Constants like this should be defined in the libc crate, not in Nix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I know I just did a PR for it tough :)

src/sys/sendfile.rs Outdated Show resolved Hide resolved

/// initialises SendfileVec to send data from `fd`.
pub fn new<F: AsFd>(
fd: F,
Copy link
Member

Choose a reason for hiding this comment

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

We should probably use BorrowedFd<'a> here so that it is consistent with FdSet and PollFd

BTW, I think we should also do this to Epoll


const SFV_FD_SELF: i32 = -2;

impl<'a> SendfileVec<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it would be great if we rename this lifetime parameter to 'fd given that libc::sendfilevec_t.sfv_fd is a file descriptor

Copy link
Contributor Author

@devnexen devnexen Nov 27, 2023

Choose a reason for hiding this comment

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

We should probably use BorrowedFd<'a> here so that it is consistent with FdSet and PollFd

BTW, I think we should also do this to Epoll

You re seing an old version.

Copy link
Member

Choose a reason for hiding this comment

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

Emmm, why did you revert the definition of SendfileVec, for that BorrowedFd, I was just suggesting changing the function parameter type to BorrowedFd<'fd>

Copy link
Member

Choose a reason for hiding this comment

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

Just like what we did to FdSet:

nix/src/sys/select.rs

Lines 18 to 47 in 39ad47b

pub struct FdSet<'fd> {
set: libc::fd_set,
_fd: std::marker::PhantomData<BorrowedFd<'fd>>,
}
fn assert_fd_valid(fd: RawFd) {
assert!(
usize::try_from(fd).map_or(false, |fd| fd < FD_SETSIZE),
"fd must be in the range 0..FD_SETSIZE",
);
}
impl<'fd> FdSet<'fd> {
/// Create an empty `FdSet`
pub fn new() -> FdSet<'fd> {
let mut fdset = mem::MaybeUninit::uninit();
unsafe {
libc::FD_ZERO(fdset.as_mut_ptr());
Self {
set: fdset.assume_init(),
_fd: std::marker::PhantomData,
}
}
}
/// Add a file descriptor to an `FdSet`
pub fn insert(&mut self, fd: BorrowedFd<'fd>) {
assert_fd_valid(fd.as_raw_fd());
unsafe { libc::FD_SET(fd.as_raw_fd(), &mut self.set) };
}

@SteveLauC
Copy link
Member

Add label W-blocking-upstream cause it is blocked by libc#3452

@devnexen
Copy link
Contributor Author

libc#3452

Do we need to tough ? I can just use directly the value if that s a problem.

@SteveLauC
Copy link
Member

Do we need to tough ?

Could you please elaborate on this? I don't think I fully understand this sentence

@devnexen
Copy link
Contributor Author

Do we need to tough ?

Could you please elaborate on this? I don't think I fully understand this sentence

I meant blocking because of libc.

@SteveLauC
Copy link
Member

Do we need to tough ?

Could you please elaborate on this? I don't think I fully understand this sentence

I meant blocking because of libc.

Nix does not define these constants ourselves, we add them to libc, and wait for a release of libc with the patch included, then we merge the Nix PR

@devnexen
Copy link
Contributor Author

Do we need to tough ?

Could you please elaborate on this? I don't think I fully understand this sentence

I meant blocking because of libc.

Nix does not define these constants ourselves, we add them to libc, and wait for a release of libc with the patch included, then we merge the Nix PR

I no longer define this constant but just the value directly for the time being. but if you insist on waiting the next libc crate release, fine by me.

@SteveLauC
Copy link
Member

I no longer define this constant but just the value directly for the time being. but if you insist on waiting the next libc crate release, fine by me.

Yes, we should wait for the libc release rather than using a magic number here

@SteveLauC
Copy link
Member

SteveLauC commented Dec 4, 2023

Hi @devnexen, We now use the libc dependency from git, so you use the libc constant in this PR

Comment on lines 143 to 149
/// initialises SendfileVec to send data from `fd`.
pub fn new<F: AsFd>(
fd: &'fd F,
off: off_t,
len: usize
) -> SendfileVec<'fd> {
let bfd = fd.as_fd();
Self{raw: libc::sendfilevec_t{sfv_fd: bfd.as_raw_fd(), sfv_flag: 0, sfv_off:off, sfv_len: len}, phantom: PhantomData}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// initialises SendfileVec to send data from `fd`.
pub fn new<F: AsFd>(
fd: &'fd F,
off: off_t,
len: usize
) -> SendfileVec<'fd> {
let bfd = fd.as_fd();
Self{raw: libc::sendfilevec_t{sfv_fd: bfd.as_raw_fd(), sfv_flag: 0, sfv_off:off, sfv_len: len}, phantom: PhantomData}
}
/// initialises SendfileVec to send data from `fd`.
pub fn new(
fd: BorrowedFd<'fd>,
off: off_t,
len: usize
) -> SendfileVec<'fd> {
let bfd = fd.as_fd();
Self{raw: libc::sendfilevec_t{sfv_fd: fd.as_raw_fd(), sfv_flag: 0, sfv_off:off, sfv_len: len}, phantom: PhantomData}
}

We should use a BorrowedFd<'fd> here to be consistent with our other interfaces

@devnexen
Copy link
Contributor Author

devnexen commented Dec 4, 2023

Hi @devnexen, We now use the libc dependency from git, so you use the libc constant in this PR

hmmm ok sure.

@SteveLauC SteveLauC added this pull request to the merge queue Dec 4, 2023
Merged via the queue into nix-rust:master with commit da45140 Dec 4, 2023
34 checks passed
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

Successfully merging this pull request may close these issues.

3 participants