nix icon indicating copy to clipboard operation
nix copied to clipboard

Add wrappers for posix_spawn related functions.

Open kosayoda opened this issue 2 years ago • 5 comments
trafficstars

Hello! This is my first PR to nix so I'd appreciate some guidance/patience.

This PR adds support for posix_spawn* related functions. All functions are added with the exception of posix_spawnattr_{get, set}schedpolicy and posix_spawnattr_{get, set}schedparam, since those require full sched.h support not yet in nix.

Implemented functions use I/O safe versions of file descriptors.

Blockers/Notes

  • I'd like to know how, if necessary, I can test the posix_spawn-related functions.
  • Most functions return a Result, as to my understanding even though the safe wrapper will prevent most failures listed by POSIX (eg. EINVAL for posix_spawnattr_setflags), implementations may return an error for other reasons not specified by the standard. Is that a correct assumption?
  • I am uncertain as to how to correctly determine targets to support (with cfg), and if there is a way to determine that locally.
  • posix_spawn* currently take the args and envp arguments similar to exec* functions, with slight modification due to https://github.com/rust-lang/libc/issues/1272. Can this be fixed together with #555?

In the meantime, existing code is ready for review.

Closes #1740.

kosayoda avatar Feb 26 '23 23:02 kosayoda

CI failures seem to indicates that libc::posix_spawn does not exist on some platforms, so this needs to be gated to specific targets.

WhyNotHugo avatar Mar 08 '24 12:03 WhyNotHugo

Hi @kosayoda, thanks for your interest in contributing to Nix! I am sorry that this PR hasn't been reviewed for such a long time!

I plan to take a look at this PR in the near future (possibly 3 days later) :)

SteveLauC avatar Apr 01 '24 01:04 SteveLauC

  • I'd like to know how, if necessary, I can test the posix_spawn-related functions.

I think we can give it a test, perhaps simply spawning a child process and waiting for it would be good.

  • Most functions return a Result, as to my understanding even though the safe wrapper will prevent most failures listed by POSIX (eg. EINVAL for [posix_spawnattr_setflags]

That's exactly what Nix is for! Even though some error cases won't happen with Nix interface, we don't do any thing special about the error handling and the return type:>

(https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawnattr_setflags.html)), implementations may return an error for other reasons not specified by the standard. Is that a correct assumption?

I guess yes? Standards are just standards, implementations can always do extra stuff. Though I think we don't need to do extra job here either as Errno::result(res) will handle everything for us.

  • I am uncertain as to how to correctly determine targets to support (with cfg), and if there is a way to determine that locally.

You can test this locally if you can cross-compile for those targets. It is also ok if you don't, since our CI gets you covered

  • posix_spawn* currently take the args and envp arguments similar to exec* functions, with slight modification due to https://github.com/rust-lang/libc/issues/1272. Can this be fixed together with #555?

Emm, seems that the posix_spawn() and posix_spawnp() exposed by the libc crate are actually correct? I think the interface proposed in this PR looks good.

As for that allocation issue, do posix_spawn suffer from this? Seems that the allocation is done before invoking fork()

SteveLauC avatar Apr 06 '24 23:04 SteveLauC

BTW, a CHANGELOG entry is needed, please see this on how to add one:)

SteveLauC avatar Apr 06 '24 23:04 SteveLauC

@kosayoda are you still working on this / would you mind if I tried picking it up?

CameronNemo avatar Jun 09 '24 05:06 CameronNemo