Add waitid and related constants and types. #489

zackw · 2017-01-06T17:10:20Z

waitid is a variation on waitpid with a marginally more convenient way of reporting the status, and a couple of handy
additional features, such as the ability to peek at an exit status without consuming it. It's in POSIX.1-2008 and should be available on all supported Unixes.

Along with it come the type idtype_t and the constants WEXITED, WSTOPPED, WCONTINUED, and WNOWAIT. The constants were already defined for unix/notbsd platforms.

The patch is currently incomplete: I'm pushing it to get CI to test platforms I don't have. Todo list is

Add a definition of siginfo_t to all platforms that don't have it.
Verify that the new constants are consistent for all *BSD platforms.
Verify that idtype_t is consistent across the board.
Add link_name annotations for waitid if/as necessary.

waitid() is a variation on waitpid() with a marginally more convenient way of reporting the status, and a couple of handy additional features, such as the ability to peek at an exit status without consuming it. It's in POSIX.1-2008 and should be available on all supported Unixes. Along with it come the type 'idtype_t' and the constants WEXITED, WSTOPPED, WCONTINUED, and WNOWAIT. Theconstants were alre dy defined for unix/notbsd platforms. Patch incomplete: several targets are going to have to add definitions of siginfo_t, but I'm not sure which ones yet.

rust-highfive · 2017-01-06T17:10:31Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton · 2017-01-06T17:27:42Z

Nice! We tend to avoid literal enum types so could that be expanded to a list of constants?

zackw · 2017-01-06T17:35:22Z

@alexcrichton The trouble is that idtype_t is actually specified as an enum on the C side. Is it even possible to express that any other way? (See http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_wait.h.html )

alexcrichton · 2017-01-06T17:38:18Z

Sure yeah, but C enums are different than Rust enums, they're extensible.

* idtype_t no longer an enum. * Darwin/x86-32 needs the $UNIX2003 thing. * Darwin, FreeBSD, and NetBSD all have different values for the new constants. * OpenBSD doesn't have this feature at all. (Hopefully we can get away with defining idtype_t anyway.)

alexcrichton · 2017-01-06T23:22:11Z

src/unix/mod.rs

@@ -447,6 +458,11 @@ extern {
               link_name = "waitpid$UNIX2003")]
    pub fn waitpid(pid: pid_t, status: *mut ::c_int, options: ::c_int)
                   -> pid_t;
+    #[cfg(not(target_os = "openbsd"))] // " if " -- appease style checker


Could the definition here be pushed down into the various modules to avoid the #[cfg]?

Can we defer that conversation until after I have something that works everywhere, please? I don't think the right tradeoff among tidiness, consistency with other stuff, and minimization of repetition will be apparent till then.

alexcrichton · 2017-01-06T23:22:26Z

src/unix/mod.rs

+// [ http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_wait.h.html ]
+// However, FFI doesn't currently know how to ABI-match a C enum
+// (rust#28925, rust#34641) and *probably* the underlying type will be
+// c_uint everywhere since all of the enumerators are representable by c_uint.


I think it's ok to remove this FIXME, I don't think it's possible to fix such a fixme.

In the new rev, it still says this, but not as a FIXME. I think it's valuable to document that we are manually matching a C enum here. Especially as, it turns out, the underlying type differs on Android.

* OpenBSD doesn't have idtype_t or the P_* constants either * FreeBSD has different values for the P_* constants * Android gives idtype_t a different signedness * Disable waitid on NetBSD as it causes a link failure - I think this may be a problem with the test environment

alexcrichton · 2017-01-07T05:41:11Z

src/unix/mod.rs

+    } else if #[cfg(target_os = "android")] {
+        pub type idtype_t = ::c_int;
+    } else {
+        pub type idtype_t = ::c_uint;


Instead of a cfg_if! block here, could the definition of this just get pushed down into respective modules?

alexcrichton · 2017-01-07T05:41:33Z

src/unix/mod.rs

@@ -204,6 +218,22 @@ pub const PRIO_MIN: ::c_int = -20;
 pub const PRIO_MAX: ::c_int = 20;

 cfg_if! {
+    if #[cfg(target_os = "openbsd")] {
+        // P_* constants are not available
+    } else if #[cfg(target_os = "freebsd")] {


Similar to above, could this cfg_if! be avoided to push the definitions into lower modules? (duplication is fine)

alexcrichton · 2017-01-07T05:42:25Z

@zackw oh if you're writing for a green bill of health on Travis before worrying about organization no worries! Feel free to just ping me when it's all working and I'll come take a look at the style.

It turns out that *only* FreeBSD and NetBSD proper have waitid, and that Solaris' additional W* constants are totally different from everyone else's, which tips me over from 'find some way to shoehorn this into the top-level unix/mod.rs' to 'put it in the submodules, live with the code duplication'.

zackw · 2017-01-08T04:13:13Z

@alexcrichton This is ready to be reviewed now. I did wind up pushing stuff down into submodules - the cross-platform variance is bigger than I thought it was. You probably want to look only at the diff versus master; let me know if you want a squashed version.

There's still a weird problem with NetBSD (which is why the actual function prototype is commented out in that file), and I'm a little surprised not to have had to add any definitions of siginfo_t.

alexcrichton · 2017-01-09T04:33:59Z

src/unix/bsd/apple/mod.rs

+// idtype_t is specified as a C enum:
+// http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_wait.h.html
+// However, FFI doesn't currently know how to ABI-match a C enum
+// (rust#28925, rust#34641).


It's ok to avoid repeating this comment in all locations, we tend to not have a lot of comments in libc due to the large amount of duplication.

Hm, the trouble is that there isn't an obvious "top" or "first" place to put it. I think I'll leave it in bsd/apple/mod.rs, which is alphabetically first, and remove it from the others; does that sound good?

Oh I think it's safe to just elide the comment in general. All the relevant properties of the typedef are already checked against the C representation, so it's basically never going to change, so it's ok to omit the comment.

alexcrichton · 2017-01-09T04:34:20Z

src/unix/bsd/netbsdlike/netbsd/mod.rs

+    // This should work, but it causes the netbsd CI build to fail with an
+    // intra-libc.a undefined reference to `wait6`.
+    //pub fn waitid(idtype: idtype_t, id: id_t, infop: *mut ::siginfo_t,
+    //              options: ::c_int) -> ::c_int;


This may just need a #[link_name] of some form, but otherwise it's ok to just delete this for now.

Yeah, OK. To be clear, though, I'm pretty sure this is a bug within (possibly just the rumprun version of) NetBSD libc. The actual error is

/usr/local/rumprun-x86_64/lib/libc.a(waitid.o): In function `_waitid': /build/rumprun/src-netbsd/lib/libc/gen/waitid.c:52: undefined reference to `wait6'

I don't think there's any way to work around that from within our code.

alexcrichton · 2017-01-09T04:34:43Z

src/unix/notbsd/mod.rs

+// (rust#28925, rust#34641).
+cfg_if! {
+    if #[cfg(target_os = "android")] {
+        pub type idtype_t = ::c_int;


Can this be pushed into the two lower modules as well to avoid the cfg_if!?

zackw · 2017-01-10T18:09:41Z

So I think this is fully baked now; do you want it rebased and squashed?

alexcrichton · 2017-01-10T18:09:51Z

@bors: r+

Thanks!

bors · 2017-01-10T18:09:52Z

📌 Commit 84bce94 has been approved by alexcrichton

bors · 2017-01-10T18:09:57Z

⌛ Testing commit 84bce94 with merge 7012cd6...

Add waitid and related constants and types. [`waitid`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/waitid.html) is a variation on `waitpid` with a marginally more convenient way of reporting the status, and a couple of handy additional features, such as the ability to peek at an exit status without consuming it. It's in POSIX.1-2008 and should be available on all supported Unixes. Along with it come the type `idtype_t` and the constants `WEXITED`, `WSTOPPED`, `WCONTINUED`, and `WNOWAIT`. The constants were already defined for unix/notbsd platforms. The patch is currently incomplete: I'm pushing it to get CI to test platforms I don't have. Todo list is * [x] Add a definition of `siginfo_t` to all platforms that don't have it. * [x] Verify that the new constants are consistent for all \*BSD platforms. * [x] Verify that `idtype_t` is consistent across the board. * [x] Add `link_name` annotations for `waitid` if/as necessary.

alexcrichton · 2017-01-10T18:17:46Z

Nah it's ok, we can go ahead and merge if CI passes

bors · 2017-01-10T19:20:00Z

💔 Test failed - status-travis

alexcrichton · 2017-01-10T19:33:34Z

@bors: retry * network errors

…

On Tue, Jan 10, 2017 at 11:20 AM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/libc/builds/190697657> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#489 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95JLbP4jsdoLgx3QFNCAeNjfEv4paks5rQ9nhgaJpZM4Lc6Re> .

bors · 2017-01-10T19:33:40Z

⌛ Testing commit 84bce94 with merge cb7f667...

Add waitid and related constants and types. [`waitid`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/waitid.html) is a variation on `waitpid` with a marginally more convenient way of reporting the status, and a couple of handy additional features, such as the ability to peek at an exit status without consuming it. It's in POSIX.1-2008 and should be available on all supported Unixes. Along with it come the type `idtype_t` and the constants `WEXITED`, `WSTOPPED`, `WCONTINUED`, and `WNOWAIT`. The constants were already defined for unix/notbsd platforms. The patch is currently incomplete: I'm pushing it to get CI to test platforms I don't have. Todo list is * [x] Add a definition of `siginfo_t` to all platforms that don't have it. * [x] Verify that the new constants are consistent for all \*BSD platforms. * [x] Verify that `idtype_t` is consistent across the board. * [x] Add `link_name` annotations for `waitid` if/as necessary.

bors · 2017-01-10T19:59:21Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing cb7f667 to master...

Fix ControlMessage::encode_into when encoding multiple messages copy_bytes updates dst so that it points after the bytes that were just copied into it. encode_into did not advance the buffer in the same way when encoding the data. See rust-lang#473

rust-highfive assigned alexcrichton Jan 6, 2017

alexcrichton reviewed Jan 6, 2017

View reviewed changes

alexcrichton reviewed Jan 7, 2017

View reviewed changes

zackw added 2 commits January 7, 2017 20:04

Patch in id_t missing in a few places

f05e48c

alexcrichton reviewed Jan 9, 2017

View reviewed changes

zackw added 2 commits January 10, 2017 11:09

Changes requested by reviewers

984fb54

Remove remaining copy of the comment about idtype_t being an enum.

84bce94

bors merged commit 84bce94 into rust-lang:master Jan 10, 2017

zackw deleted the add-waitid branch January 20, 2017 13:15

This was referenced Jan 24, 2017

Tracking issue for Child::try_wait rust-lang/rust#38903

Closed

some way to kill a running process? oconnor663/duct.rs#27

Closed

malbarbo mentioned this pull request Feb 25, 2017

Fix android SA_* constants #511

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add waitid and related constants and types. #489

Add waitid and related constants and types. #489

zackw commented Jan 6, 2017 •

edited

Loading

rust-highfive commented Jan 6, 2017

alexcrichton commented Jan 6, 2017

zackw commented Jan 6, 2017

alexcrichton commented Jan 6, 2017

alexcrichton Jan 6, 2017

zackw Jan 7, 2017

alexcrichton Jan 6, 2017

zackw Jan 7, 2017

alexcrichton Jan 7, 2017

alexcrichton Jan 7, 2017

alexcrichton commented Jan 7, 2017

zackw commented Jan 8, 2017

alexcrichton Jan 9, 2017

zackw Jan 10, 2017

alexcrichton Jan 10, 2017

zackw Jan 10, 2017

alexcrichton Jan 9, 2017

zackw Jan 10, 2017

alexcrichton Jan 9, 2017

zackw Jan 10, 2017

zackw commented Jan 10, 2017

alexcrichton commented Jan 10, 2017

bors commented Jan 10, 2017

bors commented Jan 10, 2017

alexcrichton commented Jan 10, 2017

bors commented Jan 10, 2017

alexcrichton commented Jan 10, 2017 via email

bors commented Jan 10, 2017

bors commented Jan 10, 2017

Add waitid and related constants and types. #489

Add waitid and related constants and types. #489

Conversation

zackw commented Jan 6, 2017 • edited Loading

rust-highfive commented Jan 6, 2017

alexcrichton commented Jan 6, 2017

zackw commented Jan 6, 2017

alexcrichton commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jan 7, 2017

zackw commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackw commented Jan 10, 2017

alexcrichton commented Jan 10, 2017

bors commented Jan 10, 2017

bors commented Jan 10, 2017

alexcrichton commented Jan 10, 2017

bors commented Jan 10, 2017

alexcrichton commented Jan 10, 2017 via email

bors commented Jan 10, 2017

bors commented Jan 10, 2017

zackw commented Jan 6, 2017 •

edited

Loading