Skip to content

Expand waitid support, add waitid tests #590

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

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

valpackett
Copy link
Contributor

  • add a bunch of stuff to WaitidStatus
  • detect successful return with no process more carefully according to documentation
    • Linux manpage:

      If WNOHANG was specified in options and there were no children in a waitable state, then waitid() returns 0 immediately and the state of the siginfo_t structure pointed to by infop is unspecified. To distinguish this case from that where a child was in a waitable state, zero out the si_pid field before the call and check for a nonzero value in this field after the call returns.

    • BSD manpages don't really mention the unspecified thing e.g.:

      If WNOHANG was specified and waitid() didn't find a matching process, si_signo and si_pid will be set to zero.

  • add tests
  • would it be ok to underive Copy on WaitidStatus? Technically an API break that should go to 0.38 release planning #578? But WaitidStatus was currently completely unusable with nothing on it…

@valpackett
Copy link
Contributor Author

wow, siginfo_t is such a royal mess… at least in the libc crate there are accessors, just some missing on some platforms, but for the raw linux backend… I guess we'll have to make an extension trait with accessors in backend::linux_raw::c?

@valpackett valpackett force-pushed the waitid branch 14 times, most recently from 11d8d40 to 40a9bb2 Compare April 3, 2023 05:41
@valpackett
Copy link
Contributor Author

valpackett commented Apr 3, 2023

Oofff okay, now it builds everywhere and isn't actually too bad, tho some cfg()'s are quite silly. And stuff like the linux_raw accessor for si_status should probably be pushed down into the backend modules (?)

@@ -426,7 +426,7 @@ pub(crate) fn waitid(id: WaitId<'_>, options: WaitidOptions) -> io::Result<Optio
#[cfg(not(any(target_os = "wasi", target_os = "redox", target_os = "openbsd")))]
#[inline]
fn _waitid_all(options: WaitidOptions) -> io::Result<Option<WaitidStatus>> {
let mut status = MaybeUninit::<c::siginfo_t>::uninit();
let mut status = MaybeUninit::<c::siginfo_t>::zeroed();
Copy link
Member

Choose a reason for hiding this comment

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

Is the reason for explicit zeroing here the comment in the Linux man page about zeroing out the si_pid field? If so, could you quote that documentation here, so that it's recorded in the code why this uses zeroed memory?

&self.0
}

#[cfg(linux_raw)]
Copy link
Member

Choose a reason for hiding this comment

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

Rustix does have a few places where the public API for linux_raw differs from the public API for the libc backend on Linux, but we're working on eliminating them, as it makes code less portable. Is it complicated to add Linux support using the libc backend as well? Also, yeah, your suggestion to push these details into src/backend/* sounds good; perhaps in new src/backend/*/wait.rs files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should already work with the libc backend (CI should test this, right?) — it's the final not(any(…)) version. The libc crate does make an effort to offer the same interface here on all platforms, just lacking in a few places such as rust-lang/libc#3185 and OpenBSD/NetBSD currently.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see now. I was confused by the cfgs :-).

@valpackett valpackett force-pushed the waitid branch 9 times, most recently from 6e3faad to 7155d9c Compare April 5, 2023 22:16
@valpackett
Copy link
Contributor Author

Okay, now it looks decent I guess…

@sunfishcode
Copy link
Member

Cool, thanks!

@sunfishcode sunfishcode merged commit 1a9ae58 into bytecodealliance:main Apr 5, 2023
@valpackett valpackett deleted the waitid branch April 6, 2023 03:18
@pawelchcki
Copy link

Thanks for this change. I was just looking for the missing waitidstatus methods! 🎉

@sunfishcode
Copy link
Member

This is now released in rustix 0.37.8.

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