unistd: add fexecve() #727
Conversation
src/unistd.rs
Outdated
| /// | ||
| /// If an error occurs, this function will return with an indication of the | ||
| /// cause of failure. See | ||
| /// [fexecve(3)#errors](http://man7.org/linux/man-pages/man3/fexecve.3.html#ERRORS) |
There was a problem hiding this comment.
Since fexecve is not linux-specific, could you please link to the Open Group man page instead?
|
One thing that hasn't been addressed with this functions is their use of |
src/unistd.rs
Outdated
| /// command that is run. On success, this function will not return. Instead, | ||
| /// the new program will run until it exits. | ||
| /// | ||
| /// If an error occurs, this function will return with an indication of the |
There was a problem hiding this comment.
This should be in an "Errors" section (see the book).
src/unistd.rs
Outdated
| /// [fexecve(2)#errors](http://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html#tag_16_111_05) | ||
| /// for a list of error conditions. | ||
| /// | ||
| /// This function is similar to `execve`, except that the program to be executed |
There was a problem hiding this comment.
Let's move this above the errors section.
|
@Susurrus docstring amended. |
|
I have a full example/demo at https://github.com/lucab/fxe-rs. Not sure if you want to extrapolate anything from there, as it makes more sense as part of the whole explanation. Regarding tests, I guess I can try adapting this test macro to receive the first exec parameter from the outside. @Susurrus let me know if you want me too add more stuff in here, or if it is fine already fine having an external demo/consumer for the moment. |
|
@lucab That'd be great if you could get a test working for this! The examples aren't as important, I just want something in-tree that uses this code, and having a test is the best, an example is second-best, and we're trying to avoid adding anything that doesn't include tests. |
|
@Susurrus pushed one more commit with the test (CI is mostly green by now but still ongoing). PTAL. |
|
LGTM. @asomers can you wrap up your review? Then I think we're ready to merge! |
|
There are build failures on mac because |
|
@Susurrus I feel that bad that I didn't catch this when wiring-in the libc bits. Thanks for spotting it and promptly fixing there. I've amended to exclude macos/ios, CI is greeen now. |
src/unistd.rs
Outdated
| /// cause of failure. See | ||
| /// [fexecve(2)#errors](http://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html#tag_16_111_05) | ||
| /// for a list of error conditions. | ||
| #[cfg(not(any(target_os = "macos", target_os = "ios")))] |
There was a problem hiding this comment.
We prefer to use white-lists here instead of black-lists as it makes it easier to support new platforms. Can you change this and the other ones below?
test/test_unistd.rs
Outdated
| @@ -156,6 +150,15 @@ macro_rules! execve_test_factory( | |||
| ) | |||
| ); | |||
|
|
|||
| #[cfg(not(target_os = "android"))] | |||
There was a problem hiding this comment.
This will be cleaner if you use cfg_if! and group the platform-specific code together.
|
@lucab No worries. I have a Mac, so i wanted to check that support for it was in fact missing as I assumed you didn't have access to one (most people don't). Not a big deal at all! |
|
Don't forget to squash your commits before we merge. |
This adds fexecve() to `nix::unistd`. It is available in libc since 0.2.29. Ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html
|
@Susurrus I've switched |
|
Awesome, this looks pretty solid, thanks @lucab! bors r+ |
727: unistd: add fexecve() r=Susurrus This adds fexecve(). It is available in libc since 0.2.29. Ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html
This adds fexecve(). It is available in libc since 0.2.29.
Ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html