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: string/JSON values should not be stored with NULL termination #1262

Merged
merged 4 commits into from Oct 29, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Oct 27, 2017

As described in #1261, strings were stored in the KVS with terminating NULL, but this complicates the implementation of atomic append (#1193) and the reasons for doing it no longer exist.

Drop the NULL byte.
Update tests and docs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 27, 2017

Coverage Status

Coverage increased (+0.01%) to 78.522% when pulling 66aa418 on garlick:kvs_string_noterm into aeb79c3 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 27, 2017

Codecov Report

Merging #1262 into master will increase coverage by 0.03%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master    #1262      +/-   ##
==========================================
+ Coverage   77.89%   77.92%   +0.03%     
==========================================
  Files         154      154              
  Lines       29027    29018       -9     
==========================================
+ Hits        22610    22613       +3     
+ Misses       6417     6405      -12
Impacted Files Coverage Δ
src/common/libkvs/kvs_txn.c 74.01% <100%> (ø) ⬆️
src/common/libkvs/kvs_lookup.c 77.37% <100%> (+1.67%) ⬆️
src/common/libkvs/kvs_watch.c 88.44% <100%> (+1.1%) ⬆️
src/common/libkvs/treeobj.c 83.96% <75%> (+1.1%) ⬆️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/cmd/flux-kvs.c 79.74% <0%> (-0.32%) ⬇️
src/modules/kvs/kvs.c 62.37% <0%> (-0.26%) ⬇️
src/common/libkvs/kvs.c 63.79% <0%> (-0.25%) ⬇️
src/common/libflux/message.c 81.6% <0%> (-0.24%) ⬇️
src/broker/module.c 84.07% <0%> (+0.27%) ⬆️
... and 3 more
If '-t', the RFC 11 object is displayed. '-a treeobj' causes the lookup
to be relative to an RFC 11 snapshot reference.
'key', display an error message. If no options, value is displayed directly
with a newline appended. If '-j', it is interpreted as encoded JSON and

This comment has been minimized.

@chu11

chu11 Oct 27, 2017

Contributor

should this be if value is non-zero it'll be displayed with a newline? It's a corner situation, maybe doesn't have to be mentioned.

This comment has been minimized.

@garlick

garlick Oct 27, 2017

Author Member

Good point, I'll call that out.

@@ -176,7 +176,7 @@ int flux_kvs_txn_put (flux_kvs_txn_t *txn, int flags,
}
if (validate_flags (flags, 0) < 0)
goto error;
if (!(dirent = treeobj_create_val (value, value ? strlen (value) + 1 : 0)))
if (!(dirent = treeobj_create_val (value, value ? strlen (value) : 0)))
goto error;
if (append_op_to_txn (txn, flags, key, dirent) < 0)
goto error;

This comment has been minimized.

@chu11

chu11 Oct 27, 2017

Contributor

can flux_kvs_txn_put() be collapsed into just a call to flux_kvs_txn_put_raw()?

This comment has been minimized.

@garlick

garlick Oct 27, 2017

Author Member

Yes but I didn't do that because I think it reads a bit easier if each function is variation with a common structure. I had them calling each other before to avoid code duplication and I thought it maybe was a step in the wrong direction. I don't feel too strongly though, so if you do, I don't mind changing it, LMK.

This comment has been minimized.

@chu11

chu11 Oct 27, 2017

Contributor

Nah, we can leave as is then.

}
else {
len = 0;
assert (len < buflen);

This comment has been minimized.

@chu11

chu11 Oct 27, 2017

Contributor

These asserts confused me a bit, so perhaps a comment would be useful. It seems that base64_decode_length() always includes extra buffer space? If the asserts were:

assert (len < buflen);
assert (data[len + 1] == '\0');

or

assert (len <= buflen);
assert (data[len] == '\0');

I think it would make more sense (the former would make more sense given the len == 0 check below.

This comment has been minimized.

@chu11

chu11 Oct 27, 2017

Contributor

oops, the latter would make more sense given the len == 0 check below.

This comment has been minimized.

@chu11

chu11 Oct 27, 2017

Contributor

Re-reading this, what I said may have been confusing b/c I was dumping thoughts. Let me re-write.

These asserts confused me a bit, so perhaps a comment would be useful. If the assert was

assert (len <= buflen);
assert (data[len] == '\0');

it would make sense. Perhaps base64_decode_length() always includes extra buffer space? That would make the present asserts make sense.

This comment has been minimized.

@garlick

garlick Oct 27, 2017

Author Member

I'll add some comments - I think the assertions themselves are probably right. Have a look in a sec when I push an update.

This comment has been minimized.

@chu11

chu11 Oct 27, 2017

Contributor

ahhh, len doesn't include the '\0' term. Now it makes more sense.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 27, 2017

Sorry for the noise there - I pushed my man page edit too soon.

I'll want to squash the incremental change before we merge.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 27, 2017

appeared to hit #731

FAIL: t0016-cron-faketime.t 3 - flux-cron tab works for set minute

and a build timed out, restarted it.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 28, 2017

hmm, never saw this fail in a build before

The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends --force-yes install cmake cmake-data clang-3.8 gcc-4.9 g++-4.9 libmunge-dev lua5.1 liblua5.1-0-dev luarocks uuid-dev aspell aspell-en ccache apport gdb valgrind libyaml-cpp-dev libboost-dev" failed and exited with 100 during .

Assuming a flukey bad internet connection. Restarting.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 28, 2017

assuming things pass after the last restart, I think things look good. Go ahead and squash and re-push.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2017

Coverage Status

Coverage increased (+0.05%) to 78.561% when pulling 4ee9ba4 on garlick:kvs_string_noterm into aeb79c3 on flux-framework:master.

garlick added some commits Oct 27, 2017

libkvs/treeobj: decode_val add xtra 0 byte to data
Problem: if string values are not stored in the KVS with
their NULL terminators, we need to recopy them to a len + 1
size buffer before returning to the user.

base64_decode_length() already includes provision for an
extra NULL byte.  Add some assertions to the code to
ensure (and document) that this NULL byte is present,
then document the behavior in the header.

Add a unit test.
libkvs: do not store strings with \0 term
Problem: the reasons no longer exist for storing
string/json values in the KVS with their terminating
NULL byte, and this complicates the implementation
of atomic append.

Skip storing the NULL byte, and change lookup_get()
et al to rely on the fact that treeobj_decode_val()
pads the space after the returned data with a NULL byte.

Use json_loadb() instead of _loads() when decoding
a JSON value.

Update unit and sharness tests.

Fix #1261
doc/flux_kvs_lookup(3): strings not \0 terminated
Update lookup_get_*() descriptions to reflect the fact that
string terminators are no longer stored in the KVS.

@garlick garlick force-pushed the garlick:kvs_string_noterm branch from 4ee9ba4 to 91e3738 Oct 28, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2017

Coverage Status

Coverage increased (+0.04%) to 78.553% when pulling 91e3738 on garlick:kvs_string_noterm into aeb79c3 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 28, 2017

Restarted two hung builders, one looked like it was in the python tests, another in the "issue" sharness tests. Random spots? Shoud be OK to merge now.

@chu11 chu11 merged commit 53acebc into flux-framework:master Oct 29, 2017

4 checks passed

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

@garlick garlick deleted the garlick:kvs_string_noterm branch Oct 30, 2017

@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.