From a007742a7d1aab2e6f0571eb972b2b210bb1370b Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 19 Sep 2017 11:34:54 -0700 Subject: [PATCH 1/8] modules/kvs/test: Cleanup cache tests Cleanup cache tests, placing tests in functions to make clear tests being performed. --- src/modules/kvs/test/cache.c | 287 ++++++++++++++++++++--------------- 1 file changed, 167 insertions(+), 120 deletions(-) diff --git a/src/modules/kvs/test/cache.c b/src/modules/kvs/test/cache.c index ad8bb13f462f..e5fe5e882aa1 100644 --- a/src/modules/kvs/test/cache.c +++ b/src/modules/kvs/test/cache.c @@ -15,26 +15,14 @@ void wait_cb (void *arg) (*count)++; } - -int main (int argc, char *argv[]) +void cache_tests (void) { struct cache *cache; - struct cache_entry *e1, *e2, *e3, *e4, *e5; tstat_t ts; int size, incomplete, dirty; - json_t *o1; - json_t *o2; - json_t *o3; - json_t *o4; - json_t *o; - wait_t *w; - int count; - - plan (NO_PLAN); cache_destroy (NULL); - cache_entry_destroy (NULL); - diag ("cache_destroy and cache_entry_destroy accept NULL arg"); + diag ("cache_destroy accept NULL arg"); ok ((cache = cache_create ()) != NULL, "cache_create works"); @@ -48,44 +36,67 @@ int main (int argc, char *argv[]) ok (incomplete == 0, "empty cache, incomplete == 0"); ok (dirty == 0, "empty cache, dirty == 0"); cache_destroy (cache); +} + +void cache_entry_tests (void) +{ + struct cache_entry *e; + json_t *otmp, *o1, *o2; + + cache_entry_destroy (NULL); + diag ("cache_entry_destroy accept NULL arg"); /* Play with one entry. * N.B.: json ref is NOT incremented by create or get_json. */ + o1 = json_object (); json_object_set_new (o1, "foo", json_integer (42)); - ok ((e1 = cache_entry_create (o1)) != NULL, + ok ((e = cache_entry_create (o1)) != NULL, "cache_entry_create works"); - ok (cache_entry_get_valid (e1) == true, + ok (cache_entry_get_valid (e) == true, "cache entry initially valid"); - ok (cache_entry_get_dirty (e1) == false, + ok (cache_entry_get_dirty (e) == false, "cache entry initially not dirty"); - ok (cache_entry_set_dirty (e1, true) == 0, + + ok (cache_entry_set_dirty (e, true) == 0, "cache_entry_set_dirty success"); - ok (cache_entry_get_dirty (e1) == true, + ok (cache_entry_get_dirty (e) == true, "cache entry succcessfully set dirty"); - ok (cache_entry_clear_dirty (e1) == 0, + ok (cache_entry_clear_dirty (e) == 0, "cache_entry_clear_dirty returns 0, b/c no waiters"); - ok (cache_entry_get_dirty (e1) == false, + ok (cache_entry_get_dirty (e) == false, "cache entry succcessfully now not dirty"); - ok (cache_entry_set_dirty (e1, true) == 0, + + ok (cache_entry_set_dirty (e, true) == 0, "cache_entry_set_dirty success"); - ok (cache_entry_get_dirty (e1) == true, + ok (cache_entry_get_dirty (e) == true, "cache entry succcessfully set dirty"); - ok (cache_entry_force_clear_dirty (e1) == 0, + ok (cache_entry_force_clear_dirty (e) == 0, "cache_entry_force_clear_dirty returns 0"); - ok (cache_entry_get_dirty (e1) == false, + ok (cache_entry_get_dirty (e) == false, "cache entry succcessfully now not dirty"); - ok ((o2 = cache_entry_get_json (e1)) != NULL, + + ok ((o2 = cache_entry_get_json (e)) != NULL, "json retrieved from cache entry"); - ok ((o = json_object_get (o2, "foo")) != NULL, + ok ((otmp = json_object_get (o2, "foo")) != NULL, "json_object_get success"); - ok (json_integer_value (o) == 42, + ok (json_integer_value (otmp) == 42, "expected json object found"); - cache_entry_set_json (e1, NULL); - ok (cache_entry_get_json (e1) == NULL, + + cache_entry_set_json (e, NULL); + ok (cache_entry_get_json (e) == NULL, "cache entry no longer has json object"); - cache_entry_destroy (e1); /* destroys o1 */ + + cache_entry_destroy (e); /* destroys o1 */ +} + +void waiter_tests (void) +{ + struct cache_entry *e; + json_t *o; + wait_t *w; + int count; /* Test cache entry waiters. * N.B. waiter is destroyed when run. @@ -93,19 +104,19 @@ int main (int argc, char *argv[]) count = 0; ok ((w = wait_create (wait_cb, &count)) != NULL, "wait_create works"); - ok ((e1 = cache_entry_create (NULL)) != NULL, + ok ((e = cache_entry_create (NULL)) != NULL, "cache_entry_create created empty object"); - ok (cache_entry_get_valid (e1) == false, + ok (cache_entry_get_valid (e) == false, "cache entry invalid, adding waiter"); - ok (cache_entry_clear_dirty (e1) < 0, + ok (cache_entry_clear_dirty (e) < 0, "cache_entry_clear_dirty returns error, b/c no object set"); - o1 = json_object (); - json_object_set_new (o1, "foo", json_integer (42)); - ok (cache_entry_wait_valid (e1, w) == 0, + o = json_object (); + json_object_set_new (o, "foo", json_integer (42)); + ok (cache_entry_wait_valid (e, w) == 0, "cache_entry_wait_valid success"); - ok (cache_entry_set_json (e1, o1) == 0, + ok (cache_entry_set_json (e, o) == 0, "cache_entry_set_json success"); - ok (cache_entry_get_valid (e1) == true, + ok (cache_entry_get_valid (e) == true, "cache entry set valid with one waiter"); ok (count == 1, "waiter callback ran"); @@ -113,17 +124,17 @@ int main (int argc, char *argv[]) count = 0; ok ((w = wait_create (wait_cb, &count)) != NULL, "wait_create works"); - ok (cache_entry_set_dirty (e1, true) == 0, + ok (cache_entry_set_dirty (e, true) == 0, "cache_entry_set_dirty success"); - ok (cache_entry_get_dirty (e1) == true, + ok (cache_entry_get_dirty (e) == true, "cache entry set dirty, adding waiter"); - ok (cache_entry_wait_notdirty (e1, w) == 0, + ok (cache_entry_wait_notdirty (e, w) == 0, "cache_entry_wait_notdirty success"); - ok (cache_entry_clear_dirty (e1) == 1, + ok (cache_entry_clear_dirty (e) == 1, "cache_entry_clear_dirty returns 1, b/c of a waiter"); - ok (cache_entry_set_dirty (e1, false) == 0, + ok (cache_entry_set_dirty (e, false) == 0, "cache_entry_set_dirty success"); - ok (cache_entry_get_dirty (e1) == false, + ok (cache_entry_get_dirty (e) == false, "cache entry set not dirty with one waiter"); ok (count == 1, "waiter callback ran"); @@ -131,19 +142,110 @@ int main (int argc, char *argv[]) count = 0; ok ((w = wait_create (wait_cb, &count)) != NULL, "wait_create works"); - ok (cache_entry_set_dirty (e1, true) == 0, + ok (cache_entry_set_dirty (e, true) == 0, "cache_entry_set_dirty success"); - ok (cache_entry_get_dirty (e1) == true, + ok (cache_entry_get_dirty (e) == true, "cache entry set dirty, adding waiter"); - ok (cache_entry_wait_notdirty (e1, w) == 0, + ok (cache_entry_wait_notdirty (e, w) == 0, "cache_entry_wait_notdirty success"); - ok (cache_entry_force_clear_dirty (e1) == 0, + ok (cache_entry_force_clear_dirty (e) == 0, "cache_entry_clear_dirty returns 0 w/ waiter"); - ok (cache_entry_get_dirty (e1) == false, + ok (cache_entry_get_dirty (e) == false, "cache entry set not dirty with one waiter"); ok (count == 0, "waiter callback not called on force clear dirty"); - cache_entry_destroy (e1); /* destroys o1 */ + + cache_entry_destroy (e); /* destroys o */ +} + +void cache_remove_entry_tests (void) +{ + struct cache *cache; + struct cache_entry *e; + json_t *o; + wait_t *w; + int count; + + ok ((cache = cache_create ()) != NULL, + "cache_create works"); + + ok ((e = cache_entry_create (NULL)) != NULL, + "cache_entry_create works"); + cache_insert (cache, "remove-ref", e); + ok (cache_lookup (cache, "remove-ref", 0) != NULL, + "cache_lookup verify entry exists"); + ok (cache_remove_entry (cache, "blalalala") == 0, + "cache_remove_entry failed on bad reference"); + ok (cache_remove_entry (cache, "remove-ref") == 1, + "cache_remove_entry removed cache entry w/o object"); + ok (cache_lookup (cache, "remove-ref", 0) == NULL, + "cache_lookup verify entry gone"); + + count = 0; + ok ((w = wait_create (wait_cb, &count)) != NULL, + "wait_create works"); + ok ((e = cache_entry_create (NULL)) != NULL, + "cache_entry_create created empty object"); + cache_insert (cache, "remove-ref", e); + ok (cache_lookup (cache, "remove-ref", 0) != NULL, + "cache_lookup verify entry exists"); + ok (cache_entry_get_valid (e) == false, + "cache entry invalid, adding waiter"); + ok (cache_entry_wait_valid (e, w) == 0, + "cache_entry_wait_valid success"); + ok (cache_remove_entry (cache, "remove-ref") == 0, + "cache_remove_entry failed on valid waiter"); + o = json_string ("foobar"); + ok (cache_entry_set_json (e, o) == 0, + "cache_entry_set_json success"); + ok (cache_entry_get_valid (e) == true, + "cache entry set valid with one waiter"); + ok (count == 1, + "waiter callback ran"); + ok (cache_remove_entry (cache, "remove-ref") == 1, + "cache_remove_entry removed cache entry after valid waiter gone"); + ok (cache_lookup (cache, "remove-ref", 0) == NULL, + "cache_lookup verify entry gone"); + + count = 0; + ok ((w = wait_create (wait_cb, &count)) != NULL, + "wait_create works"); + o = json_string ("foobar"); + ok ((e = cache_entry_create (o)) != NULL, + "cache_entry_create created empty object"); + cache_insert (cache, "remove-ref", e); + ok (cache_lookup (cache, "remove-ref", 0) != NULL, + "cache_lookup verify entry exists"); + ok (cache_entry_set_dirty (e, true) == 0, + "cache_entry_set_dirty success"); + ok (cache_remove_entry (cache, "remove-ref") == 0, + "cache_remove_entry not removed b/c dirty"); + ok (cache_entry_wait_notdirty (e, w) == 0, + "cache_entry_wait_notdirty success"); + ok (cache_remove_entry (cache, "remove-ref") == 0, + "cache_remove_entry failed on notdirty waiter"); + ok (cache_entry_set_dirty (e, false) == 0, + "cache_entry_set_dirty success"); + ok (count == 1, + "waiter callback ran"); + ok (cache_remove_entry (cache, "remove-ref") == 1, + "cache_remove_entry removed cache entry after notdirty waiter gone"); + ok (cache_lookup (cache, "remove-ref", 0) == NULL, + "cache_lookup verify entry gone"); + + cache_destroy (cache); +} + +void cache_expiration_tests (void) +{ + struct cache *cache; + struct cache_entry *e1, *e2, *e3, *e4; + tstat_t ts; + int size, incomplete, dirty; + json_t *o1; + json_t *o2; + json_t *o3; + json_t *otmp; /* Put entry in cache and test lookup, expire */ @@ -200,15 +302,15 @@ int main (int argc, char *argv[]) "cache_lookup of correct hash works (last use=42)"); ok ((o2 = cache_entry_get_json (e4)) != NULL, "cache_entry_get_json found entry"); - ok ((o = json_object_get (o2, "foo")) != NULL, + ok ((otmp = json_object_get (o2, "foo")) != NULL, "json_object_get success"); - ok (json_integer_value (o) == 42, + ok (json_integer_value (otmp) == 42, "expected json object found"); ok ((o3 = cache_lookup_and_get_json (cache, "xxx2", 0)) != NULL, "cache_lookup_and_get_json of correct hash and valid entry works"); - ok ((o = json_object_get (o3, "foo")) != NULL, + ok ((otmp = json_object_get (o3, "foo")) != NULL, "json_object_get success"); - ok (json_integer_value (o) == 42, + ok (json_integer_value (otmp) == 42, "expected json object found"); ok (cache_count_entries (cache) == 2, "cache contains 2 entries"); @@ -244,73 +346,18 @@ int main (int argc, char *argv[]) ok (cache_count_entries (cache) == 1, "cache contains 1 entry"); - /* cache_remove_entry tests */ - - ok ((e5 = cache_entry_create (NULL)) != NULL, - "cache_entry_create works"); - cache_insert (cache, "remove-ref", e5); - ok (cache_lookup (cache, "remove-ref", 0) != NULL, - "cache_lookup verify entry exists"); - ok (cache_remove_entry (cache, "blalalala") == 0, - "cache_remove_entry failed on bad reference"); - ok (cache_remove_entry (cache, "remove-ref") == 1, - "cache_remove_entry removed cache entry w/o object"); - ok (cache_lookup (cache, "remove-ref", 0) == NULL, - "cache_lookup verify entry gone"); - - count = 0; - ok ((w = wait_create (wait_cb, &count)) != NULL, - "wait_create works"); - ok ((e5 = cache_entry_create (NULL)) != NULL, - "cache_entry_create created empty object"); - cache_insert (cache, "remove-ref", e5); - ok (cache_lookup (cache, "remove-ref", 0) != NULL, - "cache_lookup verify entry exists"); - ok (cache_entry_get_valid (e5) == false, - "cache entry invalid, adding waiter"); - ok (cache_entry_wait_valid (e5, w) == 0, - "cache_entry_wait_valid success"); - ok (cache_remove_entry (cache, "remove-ref") == 0, - "cache_remove_entry failed on valid waiter"); - o4 = json_string ("foobar"); - ok (cache_entry_set_json (e5, o4) == 0, - "cache_entry_set_json success"); - ok (cache_entry_get_valid (e5) == true, - "cache entry set valid with one waiter"); - ok (count == 1, - "waiter callback ran"); - ok (cache_remove_entry (cache, "remove-ref") == 1, - "cache_remove_entry removed cache entry after valid waiter gone"); - ok (cache_lookup (cache, "remove-ref", 0) == NULL, - "cache_lookup verify entry gone"); + cache_destroy (cache); +} - count = 0; - ok ((w = wait_create (wait_cb, &count)) != NULL, - "wait_create works"); - o4 = json_string ("foobar"); - ok ((e5 = cache_entry_create (o4)) != NULL, - "cache_entry_create created empty object"); - cache_insert (cache, "remove-ref", e5); - ok (cache_lookup (cache, "remove-ref", 0) != NULL, - "cache_lookup verify entry exists"); - ok (cache_entry_set_dirty (e5, true) == 0, - "cache_entry_set_dirty success"); - ok (cache_remove_entry (cache, "remove-ref") == 0, - "cache_remove_entry not removed b/c dirty"); - ok (cache_entry_wait_notdirty (e5, w) == 0, - "cache_entry_wait_notdirty success"); - ok (cache_remove_entry (cache, "remove-ref") == 0, - "cache_remove_entry failed on notdirty waiter"); - ok (cache_entry_set_dirty (e5, false) == 0, - "cache_entry_set_dirty success"); - ok (count == 1, - "waiter callback ran"); - ok (cache_remove_entry (cache, "remove-ref") == 1, - "cache_remove_entry removed cache entry after notdirty waiter gone"); - ok (cache_lookup (cache, "remove-ref", 0) == NULL, - "cache_lookup verify entry gone"); +int main (int argc, char *argv[]) +{ + plan (NO_PLAN); - cache_destroy (cache); + cache_tests (); + cache_entry_tests (); + waiter_tests (); + cache_expiration_tests (); + cache_remove_entry_tests (); done_testing (); return (0); From c89573741c81d10862e748e23ba5ef86fbdbb8c7 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 26 Sep 2017 10:52:21 -0700 Subject: [PATCH 2/8] modules/kvs: Fix code style mistakes --- src/modules/kvs/lookup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/modules/kvs/lookup.c b/src/modules/kvs/lookup.c index 203acacdb87d..c775da7d7c67 100644 --- a/src/modules/kvs/lookup.c +++ b/src/modules/kvs/lookup.c @@ -545,7 +545,8 @@ int lookup_set_current_epoch (lookup_t *lh, int epoch) return -1; } -int lookup_set_aux_data (lookup_t *lh, void *data) { +int lookup_set_aux_data (lookup_t *lh, void *data) +{ if (lh && lh->magic == LOOKUP_MAGIC) { lh->aux = data; return 0; From 4b9a7a01260ac329f297c10bac67e8e3b5f12a72 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 26 Sep 2017 15:13:28 -0700 Subject: [PATCH 3/8] modules/kvs/test: Fix comment errors --- src/modules/kvs/test/lookup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/kvs/test/lookup.c b/src/modules/kvs/test/lookup.c index 7a691c2207ba..8503488271df 100644 --- a/src/modules/kvs/test/lookup.c +++ b/src/modules/kvs/test/lookup.c @@ -864,7 +864,7 @@ void lookup_links (void) { /* This cache is * - * opaque_data + * valref_ref * "abcd" * * dirref3_ref From b094be3800bdd7efbee19fc7d6dd8209fa7704a4 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 19 Sep 2017 16:52:42 -0700 Subject: [PATCH 4/8] modules/kvs/test: Cleanup lookup tests Remove unnecessary parameters passed to check() and check_stall() functions. The inputs are always the same, so they can be abstracted away. --- src/modules/kvs/test/lookup.c | 132 +++++++++++++++++----------------- 1 file changed, 64 insertions(+), 68 deletions(-) diff --git a/src/modules/kvs/test/lookup.c b/src/modules/kvs/test/lookup.c index 8503488271df..879989d2621e 100644 --- a/src/modules/kvs/test/lookup.c +++ b/src/modules/kvs/test/lookup.c @@ -258,32 +258,28 @@ void check_common (lookup_t *lh, } void check (lookup_t *lh, - bool lookup_result, int get_errnum_result, json_t *get_value_result, - const char *missing_ref_result, const char *msg) { check_common (lh, - lookup_result, + true, get_errnum_result, get_value_result, - missing_ref_result, + NULL, msg, true); } void check_stall (lookup_t *lh, - bool lookup_result, int get_errnum_result, - json_t *get_value_result, const char *missing_ref_result, const char *msg) { check_common (lh, - lookup_result, + false, get_errnum_result, - get_value_result, + NULL, missing_ref_result, msg, false); @@ -319,7 +315,7 @@ void lookup_root (void) { NULL, 0)) != NULL, "lookup_create on root, no flags, works"); - check (lh, true, EISDIR, NULL, NULL, "root no flags"); + check (lh, EISDIR, NULL, "root no flags"); /* flags = FLUX_KVS_READDIR, should succeed */ ok ((lh = lookup_create (cache, @@ -330,7 +326,7 @@ void lookup_root (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create on root w/ flag = FLUX_KVS_READDIR, works"); - check (lh, true, 0, root, NULL, "root w/ FLUX_KVS_READDIR"); + check (lh, 0, root, "root w/ FLUX_KVS_READDIR"); /* flags = FLUX_KVS_TREEOBJ, should succeed */ ok ((lh = lookup_create (cache, @@ -342,7 +338,7 @@ void lookup_root (void) { FLUX_KVS_TREEOBJ)) != NULL, "lookup_create on root w/ flag = FLUX_KVS_TREEOBJ, works"); test = treeobj_create_dirref (root_ref); - check (lh, true, 0, test, NULL, "root w/ FLUX_KVS_TREEOBJ"); + check (lh, 0, test, "root w/ FLUX_KVS_TREEOBJ"); json_decref (test); cache_destroy (cache); @@ -410,7 +406,7 @@ void lookup_basic (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create on path dirref"); - check (lh, true, 0, dirref, NULL, "lookup dirref"); + check (lh, 0, dirref, "lookup dirref"); /* lookup value via valref */ ok ((lh = lookup_create (cache, @@ -422,7 +418,7 @@ void lookup_basic (void) { 0)) != NULL, "lookup_create on path dirref.valref"); test = treeobj_create_val ("abcd", 4); - check (lh, true, 0, test, NULL, "lookup dirref.valref"); + check (lh, 0, test, "lookup dirref.valref"); json_decref (test); /* lookup value via val */ @@ -435,7 +431,7 @@ void lookup_basic (void) { 0)) != NULL, "lookup_create on path dirref.val"); test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "lookup dirref.val"); + check (lh, 0, test, "lookup dirref.val"); json_decref (test); /* lookup dir via dir */ @@ -447,7 +443,7 @@ void lookup_basic (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create on path dirref.dir"); - check (lh, true, 0, dir, NULL, "lookup dirref.dir"); + check (lh, 0, dir, "lookup dirref.dir"); /* lookup symlink */ ok ((lh = lookup_create (cache, @@ -459,7 +455,7 @@ void lookup_basic (void) { FLUX_KVS_READLINK)) != NULL, "lookup_create on path dirref.symlink"); test = treeobj_create_symlink ("baz"); - check (lh, true, 0, test, NULL, "lookup dirref.symlink"); + check (lh, 0, test, "lookup dirref.symlink"); json_decref (test); /* lookup dirref treeobj */ @@ -472,7 +468,7 @@ void lookup_basic (void) { FLUX_KVS_TREEOBJ)) != NULL, "lookup_create on path dirref (treeobj)"); test = treeobj_create_dirref (dirref_ref); - check (lh, true, 0, test, NULL, "lookup dirref treeobj"); + check (lh, 0, test, "lookup dirref treeobj"); json_decref (test); /* lookup valref treeobj */ @@ -485,7 +481,7 @@ void lookup_basic (void) { FLUX_KVS_TREEOBJ)) != NULL, "lookup_create on path dirref.valref (treeobj)"); test = treeobj_create_valref (valref_ref); - check (lh, true, 0, test, NULL, "lookup dirref.valref treeobj"); + check (lh, 0, test, "lookup dirref.valref treeobj"); json_decref (test); /* lookup val treeobj */ @@ -498,7 +494,7 @@ void lookup_basic (void) { FLUX_KVS_TREEOBJ)) != NULL, "lookup_create on path dirref.val (treeobj)"); test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "lookup dirref.val treeobj"); + check (lh, 0, test, "lookup dirref.val treeobj"); json_decref (test); /* lookup dir treeobj */ @@ -510,7 +506,7 @@ void lookup_basic (void) { NULL, FLUX_KVS_TREEOBJ)) != NULL, "lookup_create on path dirref.dir (treeobj)"); - check (lh, true, 0, dir, NULL, "lookup dirref.dir treeobj"); + check (lh, 0, dir, "lookup dirref.dir treeobj"); /* lookup symlink treeobj */ ok ((lh = lookup_create (cache, @@ -522,7 +518,7 @@ void lookup_basic (void) { FLUX_KVS_TREEOBJ)) != NULL, "lookup_create on path dirref.symlink (treeobj)"); test = treeobj_create_symlink ("baz"); - check (lh, true, 0, test, NULL, "lookup dirref.symlink treeobj"); + check (lh, 0, test, "lookup dirref.symlink treeobj"); json_decref (test); cache_destroy (cache); @@ -612,7 +608,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on bad path in path"); - check (lh, true, 0, NULL, NULL, "lookup bad path"); + check (lh, 0, NULL, "lookup bad path"); /* Lookup path w/ val in middle, Not ENOENT - caller of lookup * decides what to do with entry not found */ @@ -624,7 +620,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on val in path"); - check (lh, true, 0, NULL, NULL, "lookup val in path"); + check (lh, 0, NULL, "lookup val in path"); /* Lookup path w/ valref in middle, Not ENOENT - caller of lookup * decides what to do with entry not found */ @@ -636,7 +632,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on valref in path"); - check (lh, true, 0, NULL, NULL, "lookup valref in path"); + check (lh, 0, NULL, "lookup valref in path"); /* Lookup path w/ dir in middle, should get EPERM */ ok ((lh = lookup_create (cache, @@ -647,7 +643,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on dir in path"); - check (lh, true, EPERM, NULL, NULL, "lookup dir in path"); + check (lh, EPERM, NULL, "lookup dir in path"); /* Lookup path w/ infinite link loop, should get ELOOP */ ok ((lh = lookup_create (cache, @@ -658,7 +654,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on link loop"); - check (lh, true, ELOOP, NULL, NULL, "lookup infinite links"); + check (lh, ELOOP, NULL, "lookup infinite links"); /* Lookup a dirref, but expecting a link, should get EINVAL. */ ok ((lh = lookup_create (cache, @@ -669,7 +665,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READLINK)) != NULL, "lookup_create on dirref"); - check (lh, true, EINVAL, NULL, NULL, "lookup dirref, expecting link"); + check (lh, EINVAL, NULL, "lookup dirref, expecting link"); /* Lookup a dir, but expecting a link, should get EINVAL. */ ok ((lh = lookup_create (cache, @@ -680,7 +676,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READLINK)) != NULL, "lookup_create on dir"); - check (lh, true, EINVAL, NULL, NULL, "lookup dir, expecting link"); + check (lh, EINVAL, NULL, "lookup dir, expecting link"); /* Lookup a valref, but expecting a link, should get EINVAL. */ ok ((lh = lookup_create (cache, @@ -691,7 +687,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READLINK)) != NULL, "lookup_create on valref"); - check (lh, true, EINVAL, NULL, NULL, "lookup valref, expecting link"); + check (lh, EINVAL, NULL, "lookup valref, expecting link"); /* Lookup a val, but expecting a link, should get EINVAL. */ ok ((lh = lookup_create (cache, @@ -702,7 +698,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READLINK)) != NULL, "lookup_create on val"); - check (lh, true, EINVAL, NULL, NULL, "lookup val, expecting link"); + check (lh, EINVAL, NULL, "lookup val, expecting link"); /* Lookup a dirref, but don't expect a dir, should get EISDIR. */ ok ((lh = lookup_create (cache, @@ -713,7 +709,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on dirref"); - check (lh, true, EISDIR, NULL, NULL, "lookup dirref, not expecting dirref"); + check (lh, EISDIR, NULL, "lookup dirref, not expecting dirref"); /* Lookup a dir, but don't expect a dir, should get EISDIR. */ ok ((lh = lookup_create (cache, @@ -724,7 +720,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on dir"); - check (lh, true, EISDIR, NULL, NULL, "lookup dir, not expecting dir"); + check (lh, EISDIR, NULL, "lookup dir, not expecting dir"); /* Lookup a valref, but expecting a dir, should get ENOTDIR. */ ok ((lh = lookup_create (cache, @@ -735,7 +731,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create on valref"); - check (lh, true, ENOTDIR, NULL, NULL, "lookup valref, expecting dir"); + check (lh, ENOTDIR, NULL, "lookup valref, expecting dir"); /* Lookup a val, but expecting a dir, should get ENOTDIR. */ ok ((lh = lookup_create (cache, @@ -746,7 +742,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create on val"); - check (lh, true, ENOTDIR, NULL, NULL, "lookup val, expecting dir"); + check (lh, ENOTDIR, NULL, "lookup val, expecting dir"); /* Lookup a symlink, but expecting a dir, should get ENOTDIR. */ ok ((lh = lookup_create (cache, @@ -757,7 +753,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READLINK | FLUX_KVS_READDIR)) != NULL, "lookup_create on symlink"); - check (lh, true, ENOTDIR, NULL, NULL, "lookup symlink, expecting dir"); + check (lh, ENOTDIR, NULL, "lookup symlink, expecting dir"); /* Lookup a dirref that doesn't point to a dir, should get EPERM. */ ok ((lh = lookup_create (cache, @@ -768,7 +764,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create on dirref_bad"); - check (lh, true, EPERM, NULL, NULL, "lookup dirref_bad"); + check (lh, EPERM, NULL, "lookup dirref_bad"); /* Lookup a valref that doesn't point to a base64 string, should get EPERM */ ok ((lh = lookup_create (cache, @@ -779,7 +775,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on valref_bad"); - check (lh, true, EPERM, NULL, NULL, "lookup valref_bad"); + check (lh, EPERM, NULL, "lookup valref_bad"); /* Lookup with an invalid root_ref, should get EINVAL */ ok ((lh = lookup_create (cache, @@ -790,7 +786,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on bad root_ref"); - check (lh, true, EINVAL, NULL, NULL, "lookup bad root_ref"); + check (lh, EINVAL, NULL, "lookup bad root_ref"); /* Lookup dirref with multiple blobrefs, should get EPERM */ ok ((lh = lookup_create (cache, @@ -801,7 +797,7 @@ void lookup_errors (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create on dirref_multi"); - check (lh, true, EPERM, NULL, NULL, "lookup dirref_multi"); + check (lh, EPERM, NULL, "lookup dirref_multi"); /* Lookup path w/ dirref w/ multiple blobrefs in middle, should * get EPERM */ @@ -813,7 +809,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on dirref_multi, part of path"); - check (lh, true, EPERM, NULL, NULL, "lookup dirref_multi, part of path"); + check (lh, EPERM, NULL, "lookup dirref_multi, part of path"); /* Lookup valref with multiple blobrefs, should get EPERM */ ok ((lh = lookup_create (cache, @@ -824,7 +820,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on valref_multi"); - check (lh, true, EPERM, NULL, NULL, "lookup valref_multi"); + check (lh, EPERM, NULL, "lookup valref_multi"); /* Lookup path w/ valref w/ multiple blobrefs in middle, Not * ENOENT - caller of lookup decides what to do with entry not @@ -837,7 +833,7 @@ void lookup_errors (void) { NULL, 0)) != NULL, "lookup_create on valref_multi, part of path"); - check (lh, true, 0, NULL, NULL, "lookup valref_multi, part of path"); + check (lh, 0, NULL, "lookup valref_multi, part of path"); cache_destroy (cache); } @@ -935,7 +931,7 @@ void lookup_links (void) { 0)) != NULL, "lookup_create link to val via two links"); test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "val via two links"); + check (lh, 0, test, "val via two links"); json_decref (test); /* lookup val, link is middle of path */ @@ -948,7 +944,7 @@ void lookup_links (void) { 0)) != NULL, "lookup_create link to val"); test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "dirref1.link2dirref.val"); + check (lh, 0, test, "dirref1.link2dirref.val"); json_decref (test); /* lookup valref, link is middle of path */ @@ -961,7 +957,7 @@ void lookup_links (void) { 0)) != NULL, "lookup_create link to valref"); test = treeobj_create_val ("abcd", 4); - check (lh, true, 0, test, NULL, "dirref1.link2dirref.valref"); + check (lh, 0, test, "dirref1.link2dirref.valref"); json_decref (test); /* lookup dir, link is middle of path */ @@ -973,7 +969,7 @@ void lookup_links (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create link to dir"); - check (lh, true, 0, dir, NULL, "dirref1.link2dirref.dir"); + check (lh, 0, dir, "dirref1.link2dirref.dir"); /* lookup dirref, link is middle of path */ ok ((lh = lookup_create (cache, @@ -984,7 +980,7 @@ void lookup_links (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create link to dirref"); - check (lh, true, 0, dirref3, NULL, "dirref1.link2dirref.dirref"); + check (lh, 0, dirref3, "dirref1.link2dirref.dirref"); /* lookup symlink, link is middle of path */ ok ((lh = lookup_create (cache, @@ -996,7 +992,7 @@ void lookup_links (void) { FLUX_KVS_READLINK)) != NULL, "lookup_create link to symlink"); test = treeobj_create_symlink ("dirref2.val"); - check (lh, true, 0, test, NULL, "dirref1.link2dirref.symlink"); + check (lh, 0, test, "dirref1.link2dirref.symlink"); json_decref (test); /* lookup val, link is last part in path */ @@ -1009,7 +1005,7 @@ void lookup_links (void) { 0)) != NULL, "lookup_create link to val (last part path)"); test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "dirref1.link2val"); + check (lh, 0, test, "dirref1.link2val"); json_decref (test); /* lookup valref, link is last part in path */ @@ -1022,7 +1018,7 @@ void lookup_links (void) { 0)) != NULL, "lookup_create link to valref (last part path)"); test = treeobj_create_val ("abcd", 4); - check (lh, true, 0, test, NULL, "dirref1.link2valref"); + check (lh, 0, test, "dirref1.link2valref"); json_decref (test); /* lookup dir, link is last part in path */ @@ -1034,7 +1030,7 @@ void lookup_links (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create link to dir (last part path)"); - check (lh, true, 0, dir, NULL, "dirref1.link2dir"); + check (lh, 0, dir, "dirref1.link2dir"); /* lookup dirref, link is last part in path */ ok ((lh = lookup_create (cache, @@ -1045,7 +1041,7 @@ void lookup_links (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create link to dirref (last part path)"); - check (lh, true, 0, dirref2, NULL, "dirref1.link2dirref"); + check (lh, 0, dirref2, "dirref1.link2dirref"); /* lookup symlink, link is last part in path */ ok ((lh = lookup_create (cache, @@ -1057,7 +1053,7 @@ void lookup_links (void) { FLUX_KVS_READLINK)) != NULL, "lookup_create link to symlink (last part path)"); test = treeobj_create_symlink ("dirref2.symlink"); - check (lh, true, 0, test, NULL, "dirref1.link2symlink"); + check (lh, 0, test, "dirref1.link2symlink"); json_decref (test); cache_destroy (cache); @@ -1116,7 +1112,7 @@ void lookup_alt_root (void) { 0)) != NULL, "lookup_create val w/ dirref1 root_ref"); test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "alt root val"); + check (lh, 0, test, "alt root val"); json_decref (test); /* lookup val, alt root-ref dirref2_ref */ @@ -1129,7 +1125,7 @@ void lookup_alt_root (void) { 0)) != NULL, "lookup_create val w/ dirref2 root_ref"); test = treeobj_create_val ("bar", 3); - check (lh, true, 0, test, NULL, "alt root val"); + check (lh, 0, test, "alt root val"); json_decref (test); cache_destroy (cache); @@ -1166,12 +1162,12 @@ void lookup_stall_root (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create stalltest \".\""); - check_stall (lh, false, EAGAIN, NULL, root_ref, "root \".\" stall"); + check_stall (lh, EAGAIN, root_ref, "root \".\" stall"); cache_insert (cache, root_ref, cache_entry_create (root)); /* lookup root ".", should succeed */ - check (lh, true, 0, root, NULL, "root \".\" #1"); + check (lh, 0, root, "root \".\" #1"); /* lookup root ".", now fully cached, should succeed */ ok ((lh = lookup_create (cache, @@ -1182,7 +1178,7 @@ void lookup_stall_root (void) { NULL, FLUX_KVS_READDIR)) != NULL, "lookup_create stalltest \".\""); - check (lh, true, 0, root, NULL, "root \".\" #2"); + check (lh, 0, root, "root \".\" #2"); cache_destroy (cache); } @@ -1252,18 +1248,18 @@ void lookup_stall (void) { NULL, 0)) != NULL, "lookup_create stalltest dirref1.val"); - check_stall (lh, false, EAGAIN, NULL, root_ref, "dirref1.val stall #1"); + check_stall (lh, EAGAIN, root_ref, "dirref1.val stall #1"); cache_insert (cache, root_ref, cache_entry_create (root)); /* next call to lookup, should stall */ - check_stall (lh, false, EAGAIN, NULL, dirref1_ref, "dirref1.val stall #2"); + check_stall (lh, EAGAIN, dirref1_ref, "dirref1.val stall #2"); cache_insert (cache, dirref1_ref, cache_entry_create (dirref1)); /* final call to lookup, should succeed */ test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "dirref1.val #1"); + check (lh, 0, test, "dirref1.val #1"); json_decref (test); /* lookup dirref1.val, now fully cached, should succeed */ @@ -1276,7 +1272,7 @@ void lookup_stall (void) { 0)) != NULL, "lookup_create dirref1.val"); test = treeobj_create_val ("foo", 3); - check (lh, true, 0, test, NULL, "dirref1.val #2"); + check (lh, 0, test, "dirref1.val #2"); json_decref (test); /* lookup symlink.val, should stall */ @@ -1288,13 +1284,13 @@ void lookup_stall (void) { NULL, 0)) != NULL, "lookup_create stalltest symlink.val"); - check_stall (lh, false, EAGAIN, NULL, dirref2_ref, "symlink.val stall"); + check_stall (lh, EAGAIN, dirref2_ref, "symlink.val stall"); cache_insert (cache, dirref2_ref, cache_entry_create (dirref2)); /* lookup symlink.val, should succeed */ test = treeobj_create_val ("bar", 3); - check (lh, true, 0, test, NULL, "symlink.val #1"); + check (lh, 0, test, "symlink.val #1"); json_decref (test); /* lookup symlink.val, now fully cached, should succeed */ @@ -1307,7 +1303,7 @@ void lookup_stall (void) { 0)) != NULL, "lookup_create symlink.val"); test = treeobj_create_val ("bar", 3); - check (lh, true, 0, test, NULL, "symlink.val #2"); + check (lh, 0, test, "symlink.val #2"); json_decref (test); /* lookup dirref1.valref, should stall */ @@ -1319,13 +1315,13 @@ void lookup_stall (void) { NULL, 0)) != NULL, "lookup_create stalltest dirref1.valref"); - check_stall (lh, false, EAGAIN, NULL, valref_ref, "dirref1.valref stall"); + check_stall (lh, EAGAIN, valref_ref, "dirref1.valref stall"); cache_insert (cache, valref_ref, cache_entry_create (opaque_data)); /* lookup dirref1.valref, should succeed */ test = treeobj_create_val ("abcd", 4); - check (lh, true, 0, test, NULL, "dirref1.valref #1"); + check (lh, 0, test, "dirref1.valref #1"); json_decref (test); /* lookup dirref1.valref, now fully cached, should succeed */ @@ -1338,7 +1334,7 @@ void lookup_stall (void) { 0)) != NULL, "lookup_create stalltest dirref1.valref"); test = treeobj_create_val ("abcd", 4); - check (lh, true, 0, test, NULL, "dirref1.valref #2"); + check (lh, 0, test, "dirref1.valref #2"); json_decref (test); cache_destroy (cache); From 6647596ac116647de3ec231463cdc26ae520adab Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 27 Sep 2017 06:58:20 -0700 Subject: [PATCH 5/8] modules/kvs: Cleanup usage in main module Remove unnecessary/unused parameter in load() function. --- src/modules/kvs/kvs.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/modules/kvs/kvs.c b/src/modules/kvs/kvs.c index f6358f83c01c..ddb013363574 100644 --- a/src/modules/kvs/kvs.c +++ b/src/modules/kvs/kvs.c @@ -242,8 +242,7 @@ static int content_load_request_send (kvs_ctx_t *ctx, const href_t ref) } /* Return 0 on success, -1 on error. Set stall variable appropriately */ -static int load (kvs_ctx_t *ctx, const href_t ref, wait_t *wait, json_t **op, - bool *stall) +static int load (kvs_ctx_t *ctx, const href_t ref, wait_t *wait, bool *stall) { struct cache_entry *hp = cache_lookup (ctx->cache, ref, ctx->epoch); int saved_errno, ret; @@ -287,8 +286,6 @@ static int load (kvs_ctx_t *ctx, const href_t ref, wait_t *wait, json_t **op, return 0; } - if (op) - *op = cache_entry_get_json (hp); if (stall) *stall = false; return 0; @@ -413,7 +410,7 @@ static int commit_load_cb (commit_t *c, const char *ref, void *data) struct commit_cb_data *cbd = data; bool stall; - if (load (cbd->ctx, ref, cbd->wait, NULL, &stall) < 0) { + if (load (cbd->ctx, ref, cbd->wait, &stall) < 0) { cbd->errnum = errno; flux_log_error (cbd->ctx->h, "%s: load", __FUNCTION__); return -1; @@ -731,7 +728,7 @@ static void get_request_cb (flux_t *h, flux_msg_handler_t *w, if (!(wait = wait_create_msg_handler (h, w, msg, get_request_cb, lh))) goto done; - if (load (ctx, missing_ref, wait, NULL, &stall) < 0) { + if (load (ctx, missing_ref, wait, &stall) < 0) { flux_log_error (h, "%s: load", __FUNCTION__); goto done; } @@ -839,7 +836,7 @@ static void watch_request_cb (flux_t *h, flux_msg_handler_t *w, if (!(wait = wait_create_msg_handler (h, w, msg, watch_request_cb, lh))) goto done; - if (load (ctx, missing_ref, wait, NULL, &stall) < 0) { + if (load (ctx, missing_ref, wait, &stall) < 0) { flux_log_error (h, "%s: load", __FUNCTION__); goto done; } From e54b6eac291bf4fd574c896852ef5a4e590cb6be Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Fri, 22 Sep 2017 14:02:07 -0700 Subject: [PATCH 6/8] modules/kvs/test: Add cache unit test coverage --- src/modules/kvs/test/cache.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/modules/kvs/test/cache.c b/src/modules/kvs/test/cache.c index e5fe5e882aa1..5578707dc104 100644 --- a/src/modules/kvs/test/cache.c +++ b/src/modules/kvs/test/cache.c @@ -110,6 +110,8 @@ void waiter_tests (void) "cache entry invalid, adding waiter"); ok (cache_entry_clear_dirty (e) < 0, "cache_entry_clear_dirty returns error, b/c no object set"); + ok (cache_entry_force_clear_dirty (e) < 0, + "cache_entry_force_clear_dirty returns error, b/c no object set"); o = json_object (); json_object_set_new (o, "foo", json_integer (42)); ok (cache_entry_wait_valid (e, w) == 0, From afbd82993500dd19ad8f2f344c998943832e76d3 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 26 Sep 2017 12:03:02 -0700 Subject: [PATCH 7/8] modules/kvs/lookup: Fix corner cases & add tests Add several corner case checks and add unit tests to increase coverage. --- src/modules/kvs/lookup.c | 15 +++++++++++++++ src/modules/kvs/test/lookup.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/modules/kvs/lookup.c b/src/modules/kvs/lookup.c index c775da7d7c67..2ebaa028d514 100644 --- a/src/modules/kvs/lookup.c +++ b/src/modules/kvs/lookup.c @@ -249,6 +249,17 @@ static bool walk (lookup_t *lh) lh->missing_ref = refstr; goto stall; } + + if (!treeobj_is_dir (dir)) { + /* dirref pointed to non-dir error, special case when + * root_dirent is bad, is EINVAL from user. + */ + if (wl->depth == 0 && wl->dirent == lh->root_dirent) + lh->errnum = EINVAL; + else + lh->errnum = EPERM; + goto error; + } } else { /* Unexpected dirent type */ if (treeobj_is_valref (wl->dirent) @@ -589,6 +600,10 @@ bool lookup (lookup_t *lh) lh->missing_ref = lh->root_ref; goto stall; } + if (!treeobj_is_dir (valtmp)) { + lh->errnum = EINVAL; + goto done; + } lh->val = json_incref (valtmp); } goto done; diff --git a/src/modules/kvs/test/lookup.c b/src/modules/kvs/test/lookup.c index 879989d2621e..6340efd1fabc 100644 --- a/src/modules/kvs/test/lookup.c +++ b/src/modules/kvs/test/lookup.c @@ -288,20 +288,29 @@ void check_stall (lookup_t *lh, /* lookup tests on root dir */ void lookup_root (void) { json_t *root; + json_t *opaque_data; json_t *test; struct cache *cache; lookup_t *lh; + href_t valref_ref; href_t root_ref; ok ((cache = cache_create ()) != NULL, "cache_create works"); /* This cache is + * + * valref_ref + * "abcd" * * root_ref * treeobj dir, no entries */ + opaque_data = get_json_base64_string ("abcd"); + kvs_util_json_hash ("sha1", opaque_data, valref_ref); + cache_insert (cache, valref_ref, cache_entry_create (opaque_data)); + root = treeobj_create_dir (); kvs_util_json_hash ("sha1", root, root_ref); cache_insert (cache, root_ref, cache_entry_create (root)); @@ -341,6 +350,17 @@ void lookup_root (void) { check (lh, 0, test, "root w/ FLUX_KVS_TREEOBJ"); json_decref (test); + /* flags = FLUX_KVS_READDIR, bad root_ref, should error EINVAL */ + ok ((lh = lookup_create (cache, + 1, + root_ref, + valref_ref, + ".", + NULL, + FLUX_KVS_READDIR)) != NULL, + "lookup_create on root w/ flag = FLUX_KVS_READDIR, bad root_ref, should EINVAL"); + check (lh, EINVAL, NULL, "root w/ FLUX_KVS_READDIR, bad root_ref, should EINVAL"); + cache_destroy (cache); } @@ -766,6 +786,17 @@ void lookup_errors (void) { "lookup_create on dirref_bad"); check (lh, EPERM, NULL, "lookup dirref_bad"); + /* Lookup a dirref that doesn't point to a dir, in middle of path, should get EPERM. */ + ok ((lh = lookup_create (cache, + 1, + root_ref, + root_ref, + "dirref_bad.val", + NULL, + FLUX_KVS_READDIR)) != NULL, + "lookup_create on dirref_bad, in middle of path"); + check (lh, EPERM, NULL, "lookup dirref_bad, in middle of path"); + /* Lookup a valref that doesn't point to a base64 string, should get EPERM */ ok ((lh = lookup_create (cache, 1, From f7a8984feee0a6475a2b46d999adf3696a098b01 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Fri, 22 Sep 2017 14:17:31 -0700 Subject: [PATCH 8/8] modules/kvs/cache: Return error on bad input When input is bad or not appropriate, return -1 in cache_entry_set_dirty() and cache_entry_set_json(). Add/update unit tests appropriately. --- src/modules/kvs/cache.c | 6 ++++-- src/modules/kvs/test/cache.c | 23 ++++++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/modules/kvs/cache.c b/src/modules/kvs/cache.c index 7a6a7b8d3721..daaebbfd8323 100644 --- a/src/modules/kvs/cache.c +++ b/src/modules/kvs/cache.c @@ -99,8 +99,9 @@ int cache_entry_set_dirty (struct cache_entry *hp, bool val) } } } + return 0; } - return 0; + return -1; } int cache_entry_clear_dirty (struct cache_entry *hp) @@ -155,8 +156,9 @@ int cache_entry_set_json (struct cache_entry *hp, json_t *o) json_decref (hp->o); hp->o = NULL; } + return 0; } - return 0; + return -1; } void cache_entry_destroy (void *arg) diff --git a/src/modules/kvs/test/cache.c b/src/modules/kvs/test/cache.c index 5578707dc104..144da1bbe8bb 100644 --- a/src/modules/kvs/test/cache.c +++ b/src/modules/kvs/test/cache.c @@ -43,6 +43,9 @@ void cache_entry_tests (void) struct cache_entry *e; json_t *otmp, *o1, *o2; + /* corner case tests */ + ok (cache_entry_set_json (NULL, NULL) < 0, + "cache_entry_set_json fails with bad input"); cache_entry_destroy (NULL); diag ("cache_entry_destroy accept NULL arg"); @@ -50,6 +53,23 @@ void cache_entry_tests (void) * N.B.: json ref is NOT incremented by create or get_json. */ + /* test empty cache entry */ + + ok ((e = cache_entry_create (NULL)) != NULL, + "cache_entry_create works"); + ok (cache_entry_get_valid (e) == false, + "cache entry initially non-valid"); + ok (cache_entry_get_dirty (e) == false, + "cache entry initially not dirty"); + ok (cache_entry_set_dirty (e, true) < 0, + "cache_entry_set_dirty fails b/c entry non-valid"); + ok ((otmp = cache_entry_get_json (e)) == NULL, + "cache_entry_get_json returns NULL, no json set"); + cache_entry_destroy (e); + e = NULL; + + /* test cache entry filled with json initially */ + o1 = json_object (); json_object_set_new (o1, "foo", json_integer (42)); ok ((e = cache_entry_create (o1)) != NULL, @@ -84,7 +104,8 @@ void cache_entry_tests (void) ok (json_integer_value (otmp) == 42, "expected json object found"); - cache_entry_set_json (e, NULL); + ok (cache_entry_set_json (e, NULL) == 0, + "cache_entry_set_json success"); ok (cache_entry_get_json (e) == NULL, "cache entry no longer has json object");