Skip to content
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

Add wrappers for posix_spawn related functions. #2007

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kosayoda
Copy link

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

In the meantime, existing code is ready for review.

Closes #1740.

@WhyNotHugo
Copy link

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

@SteveLauC
Copy link
Member

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 SteveLauC mentioned this pull request Apr 1, 2024
Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the late reply, I have left some comments, the PR generally looks great! Thanks for this!

src/lib.rs Outdated Show resolved Hide resolved
src/spawn.rs Show resolved Hide resolved
src/spawn.rs Show resolved Hide resolved
src/spawn.rs Show resolved Hide resolved
src/spawn.rs Show resolved Hide resolved
src/spawn.rs Outdated Show resolved Hide resolved
src/spawn.rs Show resolved Hide resolved
src/spawn.rs Outdated Show resolved Hide resolved
src/spawn.rs Outdated Show resolved Hide resolved
src/spawn.rs Outdated
}

// Specifically for use with posix_spawn and posix_spawnp.
// https://github.com/rust-lang/libc/issues/1272
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, the interfaces exposed by the libc crate are actually correct (for posix_spawn and posix_spawnp), it is the POSIX standard's interfaces that are erroneous, so I think we do not need to link to that issue, simply documenting it like this would work:

// The POSIX standard requires those `args` and `envp` to be of type `*const *mut [c_char]`, 
// but implementations won't modify them, making the `mut` type redundant. Considering this,
// Nix does not expose this mutability, but we have to change the interface when calling the 
// underlying libc interfaces , this helper function does the conversion job.
//
// SAFETY:
// It is safe to add the mutability in types as implementations won't mutable them.

And we can remove the unsafe keyword

Choose a reason for hiding this comment

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

"implementations won't mutable them" I think you mean mutate here?

Copy link
Member

Choose a reason for hiding this comment

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

"implementations won't mutable them" I think you mean mutate here?

Ahh, right, sorry for the typo:)


And we can remove the unsafe keyword

// SAFETY:
// It is safe to add the mutability in types as implementations won't mutable them.

Just realized tha we should not make this function safe but document the SAFETY section in its usage

@SteveLauC
Copy link
Member

  • 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

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
Copy link
Member

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

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.

Interest in adding an safe API for spawn and spawn_p?
4 participants