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 #1261

Closed
garlick opened this issue Oct 27, 2017 · 0 comments
Closed

kvs: string/JSON values should not be stored with NULL termination #1261

garlick opened this issue Oct 27, 2017 · 0 comments

Comments

@garlick
Copy link
Member

garlick commented Oct 27, 2017

The KVS internally represents all values as an opaque byte sequence. The server side does not interpret them in any way, however the API side has a convention about NULL terminating strings that complicates append support proposed in #1193.

flux_kvs_txn_put() stores a C string into the KVS with its terminating NULL. This allows flux_kvs_lookup_get() to directly return value without the need re-copy to a size + 1 buffer to add a NULL to it, or so went the logic at the time (now somewhat outdated).

The JSON API functions use the same convention so that a JSON value written with flux_kvs_txn_pack() can be read out in encoded form with flux_kvs_lookup_get().

This complicates append support. If string values are concatenated by repeatedly calling flux_kvs_txn_put(FLUX_KVS_APPEND), there is no mechanism for the server to remove the NULL terminators at the end of each blob.

Let's stop storing the NULL. Looks like we can just change treeobj_decode_val() so that when it de-base64's a value, it tacks on an extra NULL byte to the decode buffer without including it in the returned length. Then the value can be used direclty as a string if desired. When we call treeobj_create_val() to encode a string, we pass the string length, not string length + 1.

garlick added a commit to garlick/flux-core that referenced this issue Oct 27, 2017
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 flux-framework#1261
garlick added a commit to garlick/flux-core that referenced this issue Oct 27, 2017
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 flux-framework#1261
@chu11 chu11 closed this as completed in 7f339ee Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant