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

subprocess: use _exit() in subprocess_child #1645

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Sep 4, 2018

This fix for #1638 didn't strictly belong with either the travis-ci/testing fixes in #1644, nor the GCC 8 updates in #1642, so I'm including it in its own PR.

Problem: Use of exit(3) on fatal errors in subprocess_child() should
not be used since the child still has a copy of the parents VM, and
thus may inappropriately call atexit or other handlers.

Use _exit(2) instead to immediately terminate the forked process on
error.

Fixes flux-framework#1638
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 79.152% when pulling 7cabbd9 on grondo:issue#1638 into 4ff7866 on flux-framework:master.

@chu11
Copy link
Member

chu11 commented Sep 4, 2018

LGTM, can merge once travis passes. Just to make sure, there isn't an order to merging given your other PR?

@chu11
Copy link
Member

chu11 commented Sep 4, 2018

hmm you hit a valgarind error, similar to #1641, but also slightly different.

@grondo
Copy link
Contributor Author

grondo commented Sep 4, 2018

Just to make sure, there isn't an order to merging given your other PR?

No particular order, but it would probably be best to merge this PR, then I'll rebase #1644, then #1642 does need to go in last.

@garlick
Copy link
Member

garlick commented Sep 4, 2018

I restarted the builder that hit the valgrind error.

@chu11 chu11 merged commit 05b1961 into flux-framework:master Sep 4, 2018
@grondo
Copy link
Contributor Author

grondo commented Sep 4, 2018

Thanks @chu11 and @garlick!

@grondo grondo deleted the issue#1638 branch September 4, 2018 16:06
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