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

flux_future_error_string: return optional error string or strerror of errnum #2091

Merged
merged 3 commits into from Mar 22, 2019

Conversation

@chu11
Copy link
Contributor

commented Mar 21, 2019

Per discussion in #2075 and updated usage where I felt it could be updated. Note that I did re-order logic of some error output, but I don't believe it's an issue.

@SteVwonder given your comment in #2075 I thought the python check_future_error should be tweaked, but it ended up breaking a test when I changed it. The function flux_future_wait_for() will simply return -1 and ETIMEDOUT if you pass in a timeout of 0. So in that particular case, the future is never fulfilled with an error. So keeping if errmsg is None is still necessary.

@chu11 chu11 force-pushed the chu11:issue2075 branch from 5710430 to 6e239b5 Mar 21, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

rebased

@codecov-io

This comment has been minimized.

Copy link

commented Mar 21, 2019

Codecov Report

Merging #2091 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2091      +/-   ##
=========================================
+ Coverage   80.37%   80.4%   +0.03%     
=========================================
  Files         192     192              
  Lines       30563   30557       -6     
=========================================
+ Hits        24564   24569       +5     
+ Misses       5999    5988      -11
Impacted Files Coverage Δ
src/cmd/flux-job.c 85.54% <100%> (+0.81%) ⬆️
src/common/libflux/future.c 87.94% <100%> (+0.07%) ⬆️
src/modules/connector-local/local.c 74.66% <0%> (+1.03%) ⬆️
else if (f->result_valid) {
if (f->result.errnum_string)
return f->result.errnum_string;
return flux_strerror (f->result.errnum);

This comment has been minimized.

Copy link
@garlick

garlick Mar 21, 2019

Member

Is this right? Is f->result.errnum valid on this leg? Or f->result.errnum_string for that matter?

Should this leg just return "Success" to be clearer?

This comment has been minimized.

Copy link
@chu11

chu11 Mar 21, 2019

Author Contributor

Is it possible you're thinking result_valid to mean the future fulfillment was a non-error fulfillment? result_valid actually means there was fulfillment that is "legit", but it may or may not be an errored fulfillment. If it's a non-error fulfillment, errnum_string should always be NULL and errnum should be 0. Thus, flux_strerror (0) returns success.

This comment has been minimized.

Copy link
@garlick

garlick Mar 21, 2019

Member

Can it be a non-error fulfillment? It's in the else leg of a test on fatal_errnum_valid...

Edit: I mean, can that block be anything but a non-error fulfillment?

This comment has been minimized.

Copy link
@garlick

garlick Mar 21, 2019

Member

Also, that function should never return NULL if it's going to be used unconditionally in error paths. If it manages to be called when a future is not fulfilled, it should probably return a string like "future is not fulfilled".

This comment has been minimized.

Copy link
@chu11

chu11 Mar 21, 2019

Author Contributor

Edit: I mean, can that block be anything but a non-error fulfillment?

The if (f->result_valid) block? It can be a error or non-error fulfillment. If it's a non-error, the f->result.errnum is 0, leading to return of "Success" as a string.

This comment has been minimized.

Copy link
@grondo

grondo Mar 21, 2019

Contributor

This probably just needs an extra comment like

/*  Future does not have a fatal error, but may have a valid error or non-error result */

I think the "fatal_errnum" was confusing me for a bit as well...

@chu11 chu11 force-pushed the chu11:issue2075 branch from 6e239b5 to bbf6091 Mar 21, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

re-pushed with change to return "future not fulfilled" if the future wasn't fulfilled. Adjusted test & manpage appropriately.

@chu11 chu11 force-pushed the chu11:issue2075 branch from bbf6091 to 44f6da9 Mar 21, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

rebased

chu11 added 3 commits Mar 20, 2019
If an error occurred on a flux future, but the error string was not
set, return the error string from flux_strerror() instead of NULL.

Update unit tests appropriately.

Fixes #2075
Update usage of flux_future_error_string() to take advantage that
it returns strerror string of internal errnum.
Update usage of flux_future_error_string() to take advantage that
it returns strerror string of internal errnum.
@chu11 chu11 force-pushed the chu11:issue2075 branch from 44f6da9 to 4c1d2d0 Mar 22, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

re-pushed adding some comments. On NULL input, function now reports "future NULL"

@codecov-io

This comment has been minimized.

Copy link

commented Mar 22, 2019

Codecov Report

Merging #2091 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2091      +/-   ##
==========================================
- Coverage    80.3%   80.28%   -0.02%     
==========================================
  Files         195      195              
  Lines       31297    31291       -6     
==========================================
- Hits        25133    25122      -11     
- Misses       6164     6169       +5
Impacted Files Coverage Δ
src/cmd/flux-job.c 85.54% <100%> (+0.81%) ⬆️
src/common/libflux/future.c 87.94% <100%> (+0.07%) ⬆️
src/common/libflux/mrpc.c 87.74% <0%> (-1.19%) ⬇️
src/modules/connector-local/local.c 73.62% <0%> (-1.04%) ⬇️
src/common/libflux/message.c 81.51% <0%> (+0.12%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

LGTM!

@grondo grondo merged commit 1a02303 into flux-framework:master Mar 22, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.3%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +19.69% compared to 11d31c1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.