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

libsubprocess: cleanup + return error on bad cmd options #2343

Merged
merged 4 commits into from Aug 28, 2019

Conversation

@chu11
Copy link
Contributor

commented Aug 27, 2019

Peeled off from #2329, some minor cleanups.

Also, in #2333 I detected invalid options for remote subprocesses and silently removed them. With the current set of options, it could go either way on silently removing vs return error to the user. But once we add I/O options, it really should be errors returned to the user if an option doesn't apply to a remote subprocess. So I now return an error to the user when an option doesn't apply to remote subprocesses.

@chu11 chu11 force-pushed the chu11:issue2329_cleanup branch from 71388fc to 7a243c5 Aug 27, 2019
chu11 added 3 commits Aug 26, 2019
Remove -E option in call to test_echo, not used in a basic stdin test.
Add new test where stdin is closed immediately on a subprocess.  Make sure
output callbacks are only called once for EOF.
In basic error tests, create a loopback flux handle instead of
using a fake pointer for tests.
@chu11 chu11 force-pushed the chu11:issue2329_cleanup branch from 7a243c5 to d5c89c8 Aug 27, 2019
if (!p)
/* check for options that do not apply to remote subprocesses */
if (!local) {
const char *substrings[] = { "STREAM_STOP", NULL };

This comment has been minimized.

Copy link
@grondo

grondo Aug 27, 2019

Contributor

Suggestion: this might be slightly more maintainable as a separate function, and would allow encapsulation of the special errno handling for success branch of flux_cmd_find_opts. Not a showstopper though.

This comment has been minimized.

Copy link
@chu11

chu11 Aug 27, 2019

Author Contributor

good idea. Now that I think about it, I moved the check into flux_rexec(). It really belongs there more.

int ret;

if ((ret = flux_cmd_find_opts (cmd, substrings)) < 0)
goto error;

This comment has been minimized.

Copy link
@grondo

grondo Aug 27, 2019

Contributor

I had trouble seeing from the diff, but at least the comments state that < 0 is not a possible return value from flux_cmd_find_opts, so does that need to be handled here? (i.e. can you just test for if (flux_cmd_find_opts (cmd, subtrings)) to simplify this code?)

This comment has been minimized.

Copy link
@chu11

chu11 Aug 27, 2019

Author Contributor

you're right! There was a chance of ENOMEM before.

Copy link
Contributor

left a comment

Looks like good cleanup to me. Just a couple comments/questions inline.

Instead of deleting cmd options that do not apply to remote
subprocesses, return an error to the user.
@chu11 chu11 force-pushed the chu11:issue2329_cleanup branch from d5c89c8 to edfcfec Aug 27, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

re-pushed with updates given comments

@codecov-io

This comment has been minimized.

Copy link

commented Aug 28, 2019

Codecov Report

Merging #2343 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2343      +/-   ##
==========================================
+ Coverage   80.82%   80.83%   +0.01%     
==========================================
  Files         215      215              
  Lines       34254    34254              
==========================================
+ Hits        27686    27690       +4     
+ Misses       6568     6564       -4
Impacted Files Coverage Δ
src/common/libsubprocess/command.c 71.54% <100%> (+0.4%) ⬆️
src/common/libsubprocess/subprocess.c 88.54% <100%> (+0.03%) ⬆️
src/common/libflux/message.c 80.95% <0%> (ø) ⬆️
src/cmd/flux-job.c 85.84% <0%> (+0.3%) ⬆️
@grondo
grondo approved these changes Aug 28, 2019
Copy link
Contributor

left a comment

LGTM now! Seems like this is straightforward enough to merge immediately.

@grondo grondo merged commit 9c7e50b into flux-framework:master Aug 28, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.82%)
Details
codecov/project 80.83% (+0.01%) compared to 0092161
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.