waitpid() now has target pid as an enum. Resolves #840. #848

serejkus · 2018-01-27T17:22:40Z

Please, note that it is an API-breaking change. Resolves #840.

Susurrus

The only big problem I see with this is that you can actually construct an incorrect WaitTarget that already contains negative PIDs, the values should be checked when it's constructed, but I'm not certain of the idiomatic way to do this.

Additionally, there is no test code or even compile-tested example code here. It'd be good to get some of that written up so we can validate that this API looks good.

Susurrus · 2018-01-28T17:24:02Z

src/sys/wait.rs

+    ThisGroupChild,
+    /// Waiting for any child.
+    AnyChild,
+    /// Waiting for a child of process group `Pid` (NB: do not negate this value,


What does NB here mean?

It's Nota Bene. I'll replace it with "please, note" since it adds confusion.

Susurrus · 2018-01-28T17:25:01Z

src/sys/wait.rs

@@ -207,7 +233,7 @@ impl WaitStatus {
    }
 }

-pub fn waitpid<P: Into<Option<Pid>>>(pid: P, options: Option<WaitPidFlag>) -> Result<WaitStatus> {
+pub fn waitpid<P: Into<WaitTarget>>(who: P, options: Option<WaitPidFlag>) -> Result<WaitStatus> {


Can you provide docs for waitpid here? I think it's important to note in them that passing a straight Pid translates to WaitTarget::Child(Pid)

You can reference #645 so it should be pretty easy.

Susurrus · 2018-01-28T17:25:22Z

src/sys/wait.rs

@@ -232,5 +266,5 @@ pub fn waitpid<P: Into<Option<Pid>>>(pid: P, options: Option<WaitPidFlag>) -> Re
 }

 pub fn wait() -> Result<WaitStatus> {


Please add docs for this function as well.

Susurrus · 2018-01-28T17:26:01Z

src/sys/wait.rs

@@ -46,6 +48,30 @@ libc_bitflags!(
    }
 );

+/// Possible child targets of`waitpid()`.


Missing space here.

Susurrus · 2018-01-28T17:26:34Z

src/sys/wait.rs

@@ -46,6 +48,30 @@ libc_bitflags!(
    }
 );

+/// Possible child targets of`waitpid()`.
+///
+/// The most common variants for waiting are waiting


Please wrap these comments a little longer so they match the wrapping elsewhere in the file (usually 79 or 99 characters)

Susurrus · 2018-01-28T17:29:28Z

src/sys/wait.rs

+    ChildOfGroup(Pid),
+}
+
+impl From<Pid> for WaitTarget {


We probably want this to follow the mapping that waitpid uses internally, where only positive PID values are mapped to WaitTarget::Child, the other 3 mappings should also be here. This should make waitpid work safely with current code. Additionally it should make this a backwards-compatible change.

Susurrus · 2018-01-28T17:33:30Z

src/sys/wait.rs

+/// for a specific child via `WaitPid::Child(pid)` and
+/// waiting for any child via `WaitPid::AnyChild`.
+#[derive(Eq, PartialEq, Clone, Copy, Debug)]
+pub enum WaitTarget {


My only concern with this enum is that constructing it allows for negative values in places where negative values shouldn't be allowed. This should be explicitly disallowed, but I'm not certain what the most Rustic way to do this is.

serejkus · 2018-01-29T08:24:55Z

Thanks for pointing these issues, I hope I'll fix them in a week.

The only big problem I see with this is that you can actually construct an incorrect WaitTarget that already contains negative PIDs, the values should be checked when it's constructed, but I'm not certain of the idiomatic way to do this.

This was my first thought that it should be protected somehow so that no user can call it with negative pids accidentally. The second thought was "how'd they get negative pids?". fork() in nix returns Result, so it is not possible. getpid() and getppid() always succeed. waitpid() returns Result in nix. Probably, I'm missing something, but currently it is only possible to create negative pid only manually, but what's the reason behind this? On the other hand, making direct use of Pid returned from very ergonomic. Like, most uses are waiting for concrete pid or waiting for any child. Waiting for concrete pid is just a matter of

match fork() {
  ForkResult::Parent(child) => { waitpid(child, None); }
  ForkResult::Child => { ... }
}

instead of being more explicit

match fork() {
  ForkResult::Parent(child) => {
    let wait_target = WaitTarget::concrete_child(child)?;
    waitpid(wait_target, None);
  }
  ForkResult::Child => { ... }
}

What do you think about it?

And a note about idiomatic way: I think that it might be implemented like

pub enum WaitTarget {
  // same non-pub variants
}

impl WaitTarget {
  pub fn child_from_raw(pid: pid_t)-> Result<WaitTarget, BadWait> {
    // ...
  }
  pub fn child(pid: Pid) -> Result<WaitTarget, BadWait> {
    // here we have to get the innards of Pid struct somehow
  }
  // this one never fails, here might be Result, but it would introduce some inconsistency
  pub fn any_child() -> WaitTarget {
  }
  // etc
}

Susurrus · 2018-02-08T04:16:20Z

I didn't quite follow what you were proposing in your last comment. I think if we just expand the From that you already have in your current implementation to also have asserts to check if there are invalid Pid values, we'll have the ergonomic API and safety that we want.

Susurrus · 2018-04-07T17:12:34Z

@serejkus Are you planning on coming back to finish this PR?

pickfire · 2020-06-24T15:59:54Z

Any news on this?

asomers-ax · 2020-06-25T00:49:32Z

Any news on this?

Apparently not. If this issue matters to you, would you be willing to adopt the PR and finish it?

pickfire · 2020-06-25T07:48:05Z

Apparently not. If this issue matters to you, would you be willing to adopt the PR and finish it?

Yes, I guess I need to work on this since I was checking on waitpid anychild but I never found it so I used libc version in the meantime.

@serejkus

Based on nix-rust#848 by @serejkus Fix nix-rust#840

@serejkus

Based on nix-rust#848 by @serejkus Fix nix-rust#840

pickfire · 2020-06-25T16:08:37Z

I believe this can be closed. See #1262 for updated patch.

Thanks @serejkus for sending in the patch, I appreciate your efforts, I have updated it and created another pull request for it.

serejkus · 2020-06-26T20:17:26Z

thank you @pickfire and sorry @Susurrus, I totally forgot about this PR. I'm closing it since it is updated by @pickfire.

Waitpid now has target pid as an enum. Resolves nix-rust#840

126a8f8

Susurrus suggested changes Jan 28, 2018

View reviewed changes

pickfire added a commit to pickfire/nix that referenced this pull request Jun 25, 2020

Add WaitTarget for waitpid

6db9c3d

Based on nix-rust#848 by @serejkus Fix nix-rust#840

pickfire added a commit to pickfire/nix that referenced this pull request Jun 25, 2020

Add WaitTarget for waitpid, breaking change

ae5228d

Based on nix-rust#848 by @serejkus Fix nix-rust#840

pickfire mentioned this pull request Jun 25, 2020

Add WaitTarget for waitpid, breaking change #1262

Open

serejkus closed this Jun 26, 2020

serejkus deleted the waitpid-target branch June 26, 2020 20:17

		@@ -232,5 +266,5 @@ pub fn waitpid<P: Into<Option<Pid>>>(pid: P, options: Option<WaitPidFlag>) -> Re
		}

		pub fn wait() -> Result<WaitStatus> {

waitpid() now has target pid as an enum. Resolves #840. #848

waitpid() now has target pid as an enum. Resolves #840. #848

Uh oh!

Conversation

serejkus commented Jan 27, 2018

Uh oh!

Susurrus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serejkus commented Jan 29, 2018

Uh oh!

Susurrus commented Feb 8, 2018

Uh oh!

Susurrus commented Apr 7, 2018

Uh oh!

pickfire commented Jun 24, 2020

Uh oh!

asomers-ax commented Jun 25, 2020

Uh oh!

pickfire commented Jun 25, 2020

Uh oh!

pickfire commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serejkus commented Jun 26, 2020

Uh oh!

Uh oh!

pickfire commented Jun 25, 2020 •

edited

Loading