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

KVS directory walk return error fixes #1058

Merged
merged 4 commits into from May 12, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented May 11, 2017

Minor changes to allow ELOOP to be returned to user when KVS links go too deep. Add appropriate tests as well.

Remove unnecessary and potentially confusing no-op line of code.

chu11 added some commits May 11, 2017

modules/kvs: Remove unused error setting
Setting errno = ENOTDIR in walk is a no-op.  The error in the
rpc response is interpreted in later code depending on the value of
dirent or the settings of flags.
test/kvs: Add test for link depth
KVS should return error when links go too deep.

@chu11 chu11 force-pushed the chu11:kvswalkerror branch from 6be5f6d to ae51934 May 11, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 11, 2017

Coverage Status

Coverage decreased (-0.01%) to 77.93% when pulling ae51934 on chu11:kvswalkerror into 5d36372 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 11, 2017

Codecov Report

Merging #1058 into master will increase coverage by <.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1058      +/-   ##
==========================================
+ Coverage   77.68%   77.69%   +<.01%     
==========================================
  Files         148      148              
  Lines       25787    25798      +11     
==========================================
+ Hits        20033    20043      +10     
- Misses       5754     5755       +1
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 80.67% <88.88%> (+0.01%) ⬆️
src/common/libflux/handle.c 85.6% <0%> (-0.26%) ⬇️
src/common/libflux/message.c 81.66% <0%> (+0.23%) ⬆️
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 11, 2017

Coverage Status

Coverage increased (+0.002%) to 77.945% when pulling ae51934 on chu11:kvswalkerror into 5d36372 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 12, 2017

Added one more patch in PR, adding comments to explain some error paths

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 12, 2017

Coverage Status

Coverage increased (+0.005%) to 77.949% when pulling ac70215 on chu11:kvswalkerror into 5d36372 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 12, 2017

This looks good. Ideally @garlick will review, but changes here look pretty straightforward to me.

I haven't looked, but is it possible to check for appropriate error message/number in the link depth test, instead of just failure? (no big deal if it is a pain)

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 12, 2017

Right now all the KVS tests simply test for failure vs non-failure, which is of course non-ideal.

Hmmm. The tests were all previously based on the fact that we were testing against the flux-kvs tool itself, so we didn't really want the tool to behave that differently than what you'd want from a "user tool". But now that it's split off into it's own testing tool t/kvs/basic.c, we could probably just change the output or exit codes to anything we want for testing purposes.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 12, 2017

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 12, 2017

we could probably just change the output or exit codes to anything we want for testing purposes.

Great idea, but probably outside the scope of this PR. Merging per @garlick

@grondo grondo merged commit 3ed906c into flux-framework:master May 12, 2017

4 checks passed

codecov/patch 88.88% of diff hit (target 77.68%)
Details
codecov/project 77.69% (+<.01%) compared to 5d36372
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 77.949%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.