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: [cleanup / refactoring] json functions are treeobj specific #1264

Closed
chu11 opened this Issue Oct 29, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@chu11
Copy link
Contributor

commented Oct 29, 2017

As discussed in #1263, a number of functions in the KVS server are labeled as "json" (i.e. cache_entry_set_json()), however in a number of circumstances everything is now treeobj specific. Rename functions and update semantics appropriately (i.e. if need to add treeobj_validate()).

Also, it is possible some functions, such as those in kvs_util could be moved into common/libkvs/treeobj. See if it makes sense to move it in there.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

also, returns from cache_entry_set_json() should probably be const (per discussion in #1265)

@chu11 chu11 self-assigned this Nov 1, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2017

ugh, having cache_entry_set_json() return const is a bit problematic. Most of the treeobj library doesn't take const.

While we could change a few treeobj functions to take a const, many can't because json_unpack() specifically cannot take a const json_t pointer. json_unpack() can manipulate reference counters underneath.

But ... json_object_get() is const. So the unpack could be converted into a series of gets and other similar extraction calls.

Is this a path we want to go down?

@garlick

This comment has been minimized.

Copy link
Member

commented Nov 1, 2017

Since changing a cache entry would change its hash, that to me seems like strong case for const, both for documentation and safety. It seems not too bad to make the change to treeobj_unpack() unless I'm missing some subtlety. If it is a pain, we could always have treeobj_unpack() take a const but cast it away before calling json_unpack() since we are very sure we're not using it in a way that modifies the object.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2017

In case you didn't see this already, a note in the jansson API docs under json_vunpack_ex only explicitly allows casting a const json_t * object to plain json_t * if neither o nor O is used:

The first argument of all unpack functions is json_t *root instead of const json_t *root, because the use of O format specifier causes the reference count of root, or some value reachable from root, to be increased. Furthermore, the o format specifier may be used to extract a value as-is, which allows modifying the structure or contents of a value reachable from root.

If the O and o format specifiers are not used, it’s perfectly safe to cast a const json_t * variable to plain json_t * when used with these functions.

@garlick

This comment has been minimized.

Copy link
Member

commented Nov 1, 2017

Oh thanks for that.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2017

Yeah, noticed that. But we specifically do use 'o' format specifier, so casting didn't seem like the greatest idea.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2017

@garlick and I chatted, one minimal idea is we could make a const and non-const version of treeobj_unpack(). This could be used by "obvious" treeobj functions that should take const (i.e. treeobj_is_dir ()).

garlick added a commit to garlick/flux-core that referenced this issue Nov 1, 2017

libkvs/treeobj: use const on json_t where approp
Add a new internal function treeobj_peek() which accepts
a const json_t * input parameter, and produces a const json_t
*data output parameter.

Internally, treeobj_peek() calls json_unpack(), which required
the const on the input parameter to be cast away - safe since,
while we use the "o" format specifier to extract the data object,
we put a const on it before we return it to internal callers.

The treeobj_peek() function allows many functions to accept
const json_t * where before they did not.

There was other one place where a const had to be cast away.  In
treeobj_validate(), which now calls treeobj_peek() to access the
const data member, the data const had to be dropped while iterating
over its directory entries with json_object_foreach(), since
json_object_iter() does not accept a const object.  I'm hopeful
that the iterator does not actually modify the object as it seems
self-contained, and this is just an oversight in the jansson API design.

As discussed in flux-framework#1264

chu11 added a commit to chu11/flux-core that referenced this issue Nov 3, 2017

libkvs/treeobj: use const on json_t where approp
Add a new internal function treeobj_peek() which accepts
a const json_t * input parameter, and produces a const json_t
*data output parameter.

Internally, treeobj_peek() calls json_unpack(), which required
the const on the input parameter to be cast away - safe since,
while we use the "o" format specifier to extract the data object,
we put a const on it before we return it to internal callers.

The treeobj_peek() function allows many functions to accept
const json_t * where before they did not.

There was one other place where a const had to be cast away.  In
treeobj_validate(), which now calls treeobj_peek() to access the
const data member, the data const had to be dropped while iterating
over its directory entries with json_object_foreach(), since
json_object_iter() does not accept a const object.  I'm hopeful
that the iterator does not actually modify the object as it seems
self-contained, and this is just an oversight in the jansson API design.

As discussed in flux-framework#1264

chu11 added a commit to chu11/flux-core that referenced this issue Nov 3, 2017

modules/kvs: Refactor cache_entry_get_treeobj()
Return const json_t * from function, as we do not want users
to modify any json object stored in the cache.

Fixes flux-framework#1264
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

as discussed in #1269, also under consideration is whether cache entries should copy their data into the entry instead of only owning the pointer. By letting the caller keep access to the pointer, they can manipulate the contents after its been stored in the cache.

chu11 added a commit to chu11/flux-core that referenced this issue Nov 3, 2017

libkvs/treeobj: use const on json_t where approp
Add a new internal function treeobj_peek() which accepts
a const json_t * input parameter, and produces a const json_t
*data output parameter.

Internally, treeobj_peek() calls json_unpack(), which required
the const on the input parameter to be cast away - safe since,
while we use the "o" format specifier to extract the data object,
we put a const on it before we return it to internal callers.

The treeobj_peek() function allows many functions to accept
const json_t * where before they did not.

There was one other place where a const had to be cast away.  In
treeobj_validate(), which now calls treeobj_peek() to access the
const data member, the data const had to be dropped while iterating
over its directory entries with json_object_foreach(), since
json_object_iter() does not accept a const object.  I'm hopeful
that the iterator does not actually modify the object as it seems
self-contained, and this is just an oversight in the jansson API design.

As discussed in flux-framework#1264
@garlick

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

I think we agreed that this should be closed and we'll open new issues on other planned cleanup

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.