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 lua refcount fix, drop unused deprecated functions #1116

Merged
merged 4 commits into from Jul 20, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jul 20, 2017

This PR collects some odds and ends that were left out of pr #1107:

  • A fix to #1112 after @grondo verified with valgrind that it's needed (I thought might fix a problem I was looking for in #1107 but that turned out to be something else).
  • Drop some "classic" functions that have no users in sched or core: kvs_put_double() and kvs_put_boolean().
  • Drop kvs_put_treeobj() which had only one test user (test converted to flux_kvs_txn_put())

garlick added some commits Jul 19, 2017

libkvs/classic: drop kvs_put_double()
kvs_put_double() is unused - remove.
libkvs/classic: drop kvs_put_boolean()
kvs_put_boolean() is unused - remove.
libkvs/classic: drop kvs_put_treeobj()
Drop kvs_put_treeobj(), after converting the one
test user to flux_kvs_txn_put (FLUX_KVS_TREEOBJ);
bindings/lua: take a reference on watch directory
Problem: valgrind shows access to freed memory when
kvsdir object created by watch callback is garbage
collected.

When a kvs_watch() callback receives a kvsdir_t
argument, it is only valid for the duration of the
callback.  When instantiating a directory as a Lua table
that persists after the callback, it looks like we need
to call kvsdir_incref() on the directory to avoid illegal
access and/or double free during garbage collection.

Fixes #1112
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 20, 2017

Coverage Status

Coverage increased (+0.005%) to 78.329% when pulling 2872e39 on garlick:kvs_misc into 3bd59f3 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 20, 2017

Codecov Report

Merging #1116 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1116      +/-   ##
==========================================
+ Coverage   78.05%   78.06%   +<.01%     
==========================================
  Files         159      159              
  Lines       26232    26220      -12     
==========================================
- Hits        20476    20468       -8     
+ Misses       5756     5752       -4
Impacted Files Coverage Δ
src/common/libkvs/kvs_classic.c 87.69% <ø> (+3.15%) ⬆️
src/bindings/lua/flux-lua.c 81.61% <100%> (ø) ⬆️
src/common/libkvs/kvs_watch.c 86.76% <0%> (-0.99%) ⬇️
src/common/libutil/dirwalk.c 93.33% <0%> (-0.75%) ⬇️
src/common/libflux/future.c 87.16% <0%> (-0.54%) ⬇️
src/common/libflux/handle.c 83.67% <0%> (-0.26%) ⬇️
src/common/libflux/message.c 81.45% <0%> (-0.12%) ⬇️
src/common/libflux/tagpool.c 96.34% <0%> (+1.21%) ⬆️
src/common/libkvs/jansson_dirent.c 75% <0%> (+1.66%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 20, 2017

Looks good to me, if you've verified the dropped functions aren't used in flux-sched/master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 20, 2017

Yep.

$ find flux-sched -name \*.c |xargs grep kvs_put_double
$ find flux-sched -name \*.c |xargs grep kvs_put_treeobj
$ find flux-sched -name \*.c |xargs grep kvs_put_boolean
$

@grondo grondo merged commit 0673cf2 into flux-framework:master Jul 20, 2017

4 checks passed

codecov/patch 100% of diff hit (target 78.05%)
Details
codecov/project 78.06% (+<.01%) compared to 3bd59f3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 78.329%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

@garlick garlick deleted the garlick:kvs_misc branch Sep 6, 2017

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.