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: do not store NUL byte on json encoded strings in kvs server #1263

Merged
merged 2 commits into from Oct 29, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Oct 28, 2017

This is the server equivalent of PR #1262 / issue #1261. Although not necessary, PR #1262 should probably be merged first.

@chu11 chu11 force-pushed the chu11:issue1261-server branch from 86bd207 to 85815ba Oct 28, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 28, 2017

Codecov Report

Merging #1263 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
+ Coverage   77.88%   77.92%   +0.04%     
==========================================
  Files         154      154              
  Lines       29018    29018              
==========================================
+ Hits        22601    22613      +12     
+ Misses       6417     6405      -12
Impacted Files Coverage Δ
src/modules/kvs/cache.c 88.72% <100%> (ø) ⬆️
src/modules/kvs/kvs_util.c 87.75% <100%> (ø) ⬆️
src/modules/kvs/commit.c 76.53% <100%> (ø) ⬆️
src/common/libutil/blobref.c 97.22% <0%> (-1.39%) ⬇️
src/common/libflux/request.c 88.46% <0%> (-1.29%) ⬇️
src/common/libutil/dirwalk.c 94.28% <0%> (-0.72%) ⬇️
src/broker/overlay.c 73.88% <0%> (-0.32%) ⬇️
src/common/libflux/message.c 81.72% <0%> (+0.23%) ⬆️
src/common/libflux/future.c 89.25% <0%> (+0.46%) ⬆️
src/common/libutil/base64.c 95.77% <0%> (+0.7%) ⬆️
... and 3 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2017

Coverage Status

Coverage increased (+0.007%) to 78.518% when pulling 85815ba on chu11:issue1261-server into aeb79c3 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 28, 2017

Looks good.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 28, 2017

One side thought (not necessarily for this PR): on the server side, the only kind of JSON stored by the cache is RFC 11 objects. Should the _json methods be renamed to _treeobj, and the functions for encoding/decoding/hashing moved from kvs_util to libkvs/treeobj?

There are already encode/decode functions in libkvs/treeobj although they use NULL terminated strings so we might need a variant that can work on unterminated buffers...

Anyway, that's much more change than is proposed in this PR so maybe something for later?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 29, 2017

Good points. I actually started to program some of what you said above, but paused when I started to include calls to treeobj_validate(), as the function semantics sort of change by making them "treeobj" specific. I'll make an issue so we don't forget.

chu11 added some commits Oct 27, 2017

modules/kvs: Do not store NUL byte
Do not store NUL byte in content store for any json encoded strings.

Update several unit tests appropriately.

@chu11 chu11 force-pushed the chu11:issue1261-server branch from 85815ba to c897c38 Oct 29, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 29, 2017

rebased and re-pushed

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 29, 2017

re-started a builder that was hung.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 29, 2017

Coverage Status

Coverage increased (+0.04%) to 78.553% when pulling c897c38 on chu11:issue1261-server into 53acebc on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 29, 2017

Great, thanks!

@garlick garlick merged commit 3a286cb into flux-framework:master Oct 29, 2017

4 checks passed

codecov/patch 100% of diff hit (target 77.88%)
Details
codecov/project 77.92% (+0.04%) compared to 53acebc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 78.553%
Details

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.