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

libflux: Fix how flux_future_error_string() obscures errors #2164

Merged
merged 5 commits into from May 22, 2019

Conversation

@chu11
Copy link
Contributor

commented May 16, 2019

Per discussion in #2160

  • flux_future_error_string() now returns NULL if the future passed in is NULL, not fulfilled, or fulfilled with a non-error result. The last one was not discussed in the issue, but after making changes, it no longer felt right that flux_future_error_string() return "Success" on a future fulfilled with a normal result. We can debate on this.

  • Added a flux_future_has_error().

  • Added FLUX_FUTURE_STRERROR(), which wraps a call to flux_future_has_error() and calls flux_future_error_string() or flux_strerror (errno) accordingly.

  • Use FLUX_FUTURE_STRERROR() in code, most notably in flux-job.

@chu11 chu11 changed the title libflux: Fix how flux_future_error_string() obscurves errors libflux: Fix how flux_future_error_string() obscures errors May 17, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Oh yeah, I forgot to mention:

  • I added FLUX_FUTURE_STRERROR to future.h. Is that the right place? It could go in flog.h instead.

  • Should FLUX_FUTURE_STRERROR take only a future? Or a future and an errno? i.e.

FLUX_FUTURE_STRERROR (f)

vs

FLUX_FUTURE_STRERROR (f, errno)

I can see cases for both. The 99% case is probably the former case. But the latter makes it more explicit that it's really a macro around flux_future_error_string() and flux_strerror(). And perhaps would allow for a case when a user would want to specify a specific errno?

@garlick

This comment has been minimized.

Copy link
Member

commented May 17, 2019

I sort of like the idea of including errno there. Could we get away with something a little shorter / not namespaced since it's a macro and the library export issue isn't a factor? Also do we need pre-processor functions to be upper case? Maybe something like:

if (thing_get (f, &object) < 0)
    flux_log (h, LOG_ERR, "shenanigans: %s, future_strerror (f, errno));
@grondo

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Could we get away with something a little shorter / not namespaced since it's a macro and the library export issue isn't a factor?

I like it, though even a macro has the chance of conflicting with another header for a project pulling in flux API + something else.

I also agree including the errno was a great thought @chu11!

@grondo

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Very minor: is the commit subject for 5e0ef19 missing a word? "Do not return EINVAL ..."

@chu11 chu11 force-pushed the chu11:issue2160 branch from 2e42dec to 994f5b8 May 17, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

re-pushed, changing FLUX_FUTURE_STRERROR(f) to future_strerror(f, errno). Also fixed the nit in the commit that @grondo found.

chu11 added 5 commits May 16, 2019
For consistency to other functions that wrap around flux_rpc_get(),
do not return EINVAL when an in & out parameter is set to NULL.
When a future is NULL or is not fulfilled, return NULL instead
of a string indicating the problem.

Update documentation and unit tests appropriately.
Add new function to detect if a future contains an error in it.

Add unit tests and documentation as well.
Add macro future_strerror() which is a wrapper that checks
if a future contains an error.  If a future does, retrieve the
error string in the future.  If not, return the error string
from errno.

Update locations that previously used flux_future_error_string().

Fixes #2160
Alter logic of flux_future_error_string() to return an error string
only when the future is fulfilled with an error, may it be a result error
or a fatal error.

Update documentation and unit tests accordingly.
@chu11 chu11 force-pushed the chu11:issue2160 branch from 994f5b8 to b213912 May 17, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

rebased

@codecov-io

This comment has been minimized.

Copy link

commented May 17, 2019

Codecov Report

Merging #2164 into master will decrease coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #2164      +/-   ##
==========================================
- Coverage   80.47%   80.46%   -0.02%     
==========================================
  Files         200      200              
  Lines       31831    31840       +9     
==========================================
+ Hits        25616    25619       +3     
- Misses       6215     6221       +6
Impacted Files Coverage Δ
src/modules/job-ingest/job-ingest.c 73.84% <100%> (-0.49%) ⬇️
src/common/libflux/future.c 88.98% <100%> (+0.2%) ⬆️
src/common/libjob/job.c 76.59% <100%> (ø) ⬆️
src/cmd/flux-job.c 86.26% <66.66%> (-0.1%) ⬇️
src/common/libflux/mrpc.c 87.79% <0%> (-1.19%) ⬇️
src/common/libutil/veb.c 98.28% <0%> (-0.58%) ⬇️
@garlick

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I'm good with this as is. Ready to merge?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Yup

@garlick

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Thanks!

@garlick garlick merged commit af0ec65 into flux-framework:master May 22, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 88.88% of diff hit (target 80.47%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +8.41% compared to 15ad6f6
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.