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

Ability to optionally log #1

Merged
merged 5 commits into from
Sep 26, 2023
Merged

Ability to optionally log #1

merged 5 commits into from
Sep 26, 2023

Conversation

psibi
Copy link
Member

@psibi psibi commented Sep 22, 2023

By default it doesn't log. This also fixes the segmentation fault issue that was happening before.

@psibi psibi requested a review from snoyberg September 26, 2023 03:32
loop {
for signal in signals.pending() {
for signal in signals.forever() {
Copy link
Member Author

@psibi psibi Sep 26, 2023

Choose a reason for hiding this comment

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

This fixes the busy loop issue. Earlier the above loop caused 100% cpu consumption.

This comment was marked as duplicate.

let pid = u32::try_from(pid.as_raw()).ok()?;
if pid == child {
graceful_shutdown()
Copy link
Member Author

Choose a reason for hiding this comment

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

Calling the abort call previously resulted in the segmentation fault issue.

This comment was marked as duplicate.

}

return 0;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Sample program to test that it does reap the orphan process.

src/lib.rs Outdated Show resolved Hide resolved
let exit_status = ExitStatus {
pid,
// Translate signal to exit code
exit_code: signal as i32 + 128,

This comment was marked as duplicate.

Choose a reason for hiding this comment

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

The process can exit because of two reasons:

  • Normal exit
  • Exit via Unix signals

In case of Unix signals, we remap it to proper exit code.

Copy link
Member

Choose a reason for hiding this comment

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

I get that, but you seem to be conflating the process ID and the exit status.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think renaming it from ExitStatus to ProcessStatus will be helpful ?

Copy link
Member

Choose a reason for hiding this comment

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

Can you look over the code one more time from scratch? I think you'll see what I'm talking about: in some branches you're returning a process ID, in some an exit code, and in some a signal, but assigning all of them to a variable called pid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have gone ahead with some renaming which will hopefully help with the confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you look over the code one more time from scratch? I think you'll see what I'm talking about: in some branches you're returning a process ID, in some an exit code, and in some a signal, but assigning all of them to a variable called pid.

Sure, I will let you know once it's ready for review. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I see my confusion, you were completely correct. Sorry for the noise.

src/lib.rs Outdated
@@ -50,10 +81,12 @@ fn pid1_handling(child: Option<Child>) -> ! {
};
(|| {
let child = child?;
let pid = pid?;
let child_exit_status = pid?;

This comment was marked as duplicate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the confusing name, I have renamed it. Can you see if that helps ?

@snoyberg snoyberg merged commit 4e169e9 into master Sep 26, 2023
3 checks passed
@snoyberg snoyberg deleted the log branch September 26, 2023 05:41
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.

4 participants