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

expose si_status for SIGCHLD #166

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

Conversation

LunarLambda
Copy link

Problem: there is no way to get the exit status or signal of a child process when handling SIGCHLD.

Solution: expose siginfo_t's si_status field in Process when extracting info for SIGCHLD.

Let me know if I should drop or change the version bump commit. It might not be entirely correct.

closes #165

Problem: there is no way to get the exit status or signal of a child
process when handling SIGCHLD

Solution: expose `siginfo_t`'s `si_status` field in `Process` when
extracting info for SIGCHLD

closes vorner#165
@vorner
Copy link
Owner

vorner commented Nov 16, 2024

Apparently, the CI degraded over time, so it's failing a lot :-(. This is a change that would probably need a thorough checking over all the platforms.

How portable is this field? Is it supported on every weird platform that exists?

Also, why is it needed? When you receive a SIGCHLD, aren't you supposed to waitpid on it anyway?

As for the bump of version, it also needs a note in the changelog. And I don't think there's a need to go from 0.3 to 0.4.

@LunarLambda
Copy link
Author

LunarLambda commented Nov 16, 2024

si_status is part of POSIX. However, depending on platform it might be stuffed inside of a (anonymous) union. Hence the SIGCHLD check in extract.c

Yes, you are generally meant to wait on the process to reap then, however this allows getting all the info in one place, as well as std::process::Child only allows you to wait for exit, so you can't handle things like a child processing suspending or stopping due to job control (SIGSTOP, SIGTOUT) or similar.

I can remove the version bump and add release notes. I thought it would be necessary since it adds a new public field to Process.

@vorner
Copy link
Owner

vorner commented Nov 16, 2024

si_status is part of POSIX.

Would you have the time to look at the CI? I'm generally busy lately, and as I said, I like the change, but I'm worried about the compatibility. The fact it is defined by POSIX is a good starting point for hope, but the library already contains a few workarounds for some platforms that … have a different idea about what POSIX really is.

I thought it would be necessary since it adds a new public field to Process.

Yes, it does. But fortunately, Process is marked as non-exhaustive which prevents someone from outside of the crate from creating it by listing all the fields ‒ which ensures backwards compatibility while adding new fields.

@LunarLambda
Copy link
Author

I had missed the struct being non_exhaustive, that's good then.

I can take a look at the CI, but I don't have a ton of experience with that. I did run cargo test over the whole workspace at least.

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.

Expose siginfo_t si_status field as Option<c_int> in siginfo::Process on SIGCHLD
2 participants