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

Fix racey behavior in job log test + minor cleanups #942

Merged
merged 4 commits into from Jan 10, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Jan 7, 2017

Minor cleanups mostly to check for errors on strtoul() and strtod() calls, plus a typo error I found in the broker.

@garlick garlick added the review label Jan 7, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 7, 2017

Current coverage is 76.06% (diff: 26.31%)

Merging #942 into master will decrease coverage by <.01%

@@             master       #942   diff @@
==========================================
  Files           149        149          
  Lines         25951      25962    +11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19742      19748     +6   
- Misses         6209       6214     +5   
  Partials          0          0          
Diff Coverage File Path
0% src/cmd/flux-comms-stats.c
•••• 45% src/broker/broker.c

Powered by Codecov. Last update f172cc3...9ee7a4b

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 7, 2017

Coverage Status

Coverage decreased (-0.006%) to 76.352% when pulling 201e882 on chu11:issue938 into f172cc3 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 7, 2017

Code coverage does go down due to missing error paths of new checks.

@chu11 chu11 force-pushed the chu11:issue938 branch from 201e882 to 36c1ce1 Jan 7, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 7, 2017

Coverage Status

Coverage increased (+0.006%) to 76.364% when pulling 36c1ce1 on chu11:issue938 into f172cc3 on flux-framework:master.

@garlick
Copy link
Member

garlick left a comment

I think you missed adding endptr to the strtoul call.

Also the errno reuslt can only happen with a zero return so we could explicitly test for that before looking at errno (The way they use errno here is pretty unorthodox), or we could skip that check and let the subsequent bounds checking take care of it since 0 is not a valid branching factor for the TBON.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 10, 2017

Strike that last comment - just saw the advice in the NOTES section of the man page

       Since  0  can legitimately be returned on both success and failure, the
       calling program should set errno to 0 before the call, and then  deter‐
       mine if an error occurred by checking whether errno has a nonzero value
       after the call.

I guess let's do that then. Meh.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 10, 2017

@garlick Ahh good eye, I did miss one endptr addition.

chu11 added some commits Jan 6, 2017

src/broker/broker.c: Fix string length bug
Fix bug in which length of rc1 was calculated instead of rc3.
src/broker/broker.c: Add cmdline error checks
Check return values from command line parsing of values using
strtoul() and strtod().
cmd/flux-comms-stats: Add cmdline error checks
Check return values from command line parsing of values using
strtoul() and strtod().
test/joblog: Fix racey behavior
On occassion joblog tests could fail due to broker shutdown
occuring too quickly and joblog not generating due to rc3
dieing prematurely.  Increase shutdown grace period to remove
racey behavior.

Fixes #938

@chu11 chu11 force-pushed the chu11:issue938 branch from 36c1ce1 to 9ee7a4b Jan 10, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 10, 2017

Coverage Status

Coverage decreased (-0.009%) to 76.349% when pulling 9ee7a4b on chu11:issue938 into f172cc3 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 10, 2017

Good catch on the rc1/rc3 typo! Ok, this looks good. Thanks!

@garlick garlick merged commit c9b4fd7 into flux-framework:master Jan 10, 2017

2 of 4 checks passed

codecov/patch 26.31% of diff hit (target 76.07%)
Details
codecov/project 76.06% (-0.01%) compared to f172cc3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 76.349%
Details

@garlick garlick removed the review label Jan 10, 2017

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.