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

pidfd_getfd, pid_open and pidfd_send_signal #1868

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JonathanWoollett-Light
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light commented Nov 16, 2022

Implemented pidfd_getfd, pid_open and pidfd_send_signal.

It seems PIDFD_NONBLOCK is missing from libc on musl toolchains: rust-lang/libc#3002.

Previously blocked on #1962, rust-lang/libc#3012 and rust-lang/libc#3026.

@JonathanWoollett-Light JonathanWoollett-Light marked this pull request as ready for review November 17, 2022 00:49
bors bot added a commit that referenced this pull request Dec 11, 2022
1923: feat: I/O safety for 'sys/wait' r=asomers a=SteveLauC

#### What this PR does:
1. Adds I/O safety for `sys/wait`

----------

Actually, I am not sure about which type to use here:
```rust
pub enum Id<'fd> {
    /// Wait for the child referred to by the given PID file descriptor
    #[cfg(any(target_os = "android", target_os = "linux"))]
    PIDFd(RawFd),
    PIDFd(BorrowedFd<'fd>),
}
```

If we use `Fd: AsFd`

```rust
pub enum Id<'fd, Fd: AsFd> {
    /// Wait for the child referred to by the given PID file descriptor
    #[cfg(any(target_os = "android", target_os = "linux"))]
    PIDFd(RawFd),
    PIDFd(&'fd Fd),
}
```

 then the user has to specify that generic type when using this interface, which is kinda user-unfriendly...


------

The typical usage of this interface will be something like:

```rust
// Thought currently we don't have pidfd_open(2) in `Nix`
let fd_referring_to_a_process: OwnedFd  = pidfd_open().unwrap();
let status = waitid(Id::PIDFd(fd_referring_to_a_process), WaitPidFlag::XXXX).unwrap();
```

UPDATE: `pidfd_open(2)` will be added in #1859 or #1868 .


Co-authored-by: Steve Lau <stevelauc@outlook.com>
bors bot added a commit that referenced this pull request Jan 11, 2023
1962: fix: Update libc r=asomers a=JonathanWoollett-Light

To include fix rust-lang/libc@44cc30c.

This is needed to unblock #1868.

Co-authored-by: Jonathan <jonathanwoollettlight@gmail.com>
Copy link

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

My previous PR contains more tests. It would be good to cherry-pick some here.

https://github.com/nix-rust/nix/pull/1859/files#diff-5743ec8a42de23a078d96881e7e34a3e9ccb68f51760f1caa73540cc021735e5

src/sys/mod.rs Outdated
@@ -226,3 +226,7 @@ feature! {
#![feature = "time"]
pub mod timer;
}

#[cfg(all(target_os = "linux", feature = "signal", feature = "process"))]
/// pidfd related functionality
Copy link

Choose a reason for hiding this comment

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

This mod is public. I think it's better to have a capitalized description inside src/sys/pidfd.rs instead, to align with other mods like memfd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, fixed.

// Wait for SIGUSR1
let mut mask = SigSet::empty();
mask.add(Signal::SIGUSR1);
assert_eq!(mask.wait().unwrap(), Signal::SIGUSR1);
Copy link

Choose a reason for hiding this comment

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

The assertion is done in child process. So even if it fails, the main process would still succeed, which is not what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the parent needs to assert on the exit code of the child process? How would you suggest doing this?

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.

None yet

2 participants