libsubprocess: invert SETPGRP flag logic #6082
Conversation
chu11
force-pushed
the
issue5968_libsubprocess_remote_subprocess_flags
branch
from
ce51c53
to
cb164ef
Compare
Problem: Some function parameters were not indented properly. Fix invalid indentation.
Problem: Almost all subprocess flags indicate if they can be used locally, remote, or both. One flag lacked this documentation. Update comment to indicate FLUX_SUBPROCESS_FLAGS_FORK_EXEC can be used locally only.
Problem: Remote subprocesses default to always set the SETPGRP flag. This leads to inconsistent behavior where local subprocesses don't have the flag set but remote ones do. We could remove this default and require remote subprocess execution to set this flag as needed. However, it ends up that the vast majority of code in flux-core sets the SETPGRP flag anyways, leading to an excess amount of code change to make local vs remote subprocesses consistent. Solution: Invert the SETPGRP flag logic into NO_SETPGRP. Now, by default, have both local and remote subprocesses call setpgrp(2). The NO_SETPGRP flag will then disable the call to setpgrp(2). Update all callers to libsubprocess accordingly. Fixes flux-framework#5968
chu11
force-pushed
the
issue5968_libsubprocess_remote_subprocess_flags
branch
from
cb164ef
to
9fe507b
Compare
API wise, NO_SETPGRP seems vaguely wrong to me, but I see where you're coming from about updating all the tests n stuff. @grondo any thoughts? |
Updating everything to set the SETPGRP flag is certainly ok and fine too, we could do that. It's mostly to get this consistent, b/c the follow up will be to allow subprocess flags to be passed to remote subprocesses. |
|
Yeah, I was struggling with NO_SETPGRP too, but the simplicity is indeed appealing. I think this PR is probably fine as is. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6082 +/- ##
==========================================
- Coverage 83.31% 83.30% -0.02%
==========================================
Files 523 523
Lines 86194 86199 +5
==========================================
- Hits 71811 71806 -5
- Misses 14383 14393 +10
|
Problem: Remote subprocesses default to always set the SETPGRP flag. This leads to inconsistent behavior where local subprocesses don't have the flag set but remote ones do.
We could remove this default and require remote subprocess execution to set this flag as needed. However, it ends up that the vast majority of code in flux-core sets the SETPGRP flag anyways, leading to an excess amount of code change to make local vs remote subprocesses consistent.
Solution: Invert the SETPGRP flag logic into NO_SETPGRP. Now, by default, have both local and remote subprocesses call setpgrp(2). The NO_SETPGRP flag will then disable the call to setpgrp(2). Update all callers to libsubprocess accordingly.
Note that we leave the new NO_SETPGRP flag to be for local subprocesses only. If there comes a time for it to be supported for remote subprocesses, that will be for another day.