From 041a5a12be5cb4864749564765a567d4b9fd92d7 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Thu, 15 Feb 2018 12:00:24 -0800 Subject: [PATCH 1/5] modules/kvs: Fix invalid tabbing --- src/modules/kvs/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/kvs/commit.c b/src/modules/kvs/commit.c index 3c1fafc84a09..a5f823d5a291 100644 --- a/src/modules/kvs/commit.c +++ b/src/modules/kvs/commit.c @@ -811,7 +811,7 @@ commit_process_t commit_process (commit_t *c, false, c->newroot, &entry)) < 0) - c->errnum = errno; + c->errnum = errno; else if (sret && zlist_push (c->dirty_cache_entries_list, entry) < 0) { commit_cleanup_dirty_cache_entry (c, entry); From 2b0634a8f30ff6b4a808dea2f5302807417291bb Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 14 Feb 2018 15:49:18 -0800 Subject: [PATCH 2/5] modules/fence: Rename fence_add_request_data() Rename to fence_add_request_ops(), which makes more sense. --- src/modules/kvs/fence.c | 2 +- src/modules/kvs/fence.h | 4 ++-- src/modules/kvs/kvs.c | 8 ++++---- src/modules/kvs/test/commit.c | 16 ++++++++-------- src/modules/kvs/test/fence.c | 24 ++++++++++++------------ src/modules/kvs/test/kvsroot.c | 2 +- 6 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/modules/kvs/fence.c b/src/modules/kvs/fence.c index 31567ab2ac9d..dc6a3852a47c 100644 --- a/src/modules/kvs/fence.c +++ b/src/modules/kvs/fence.c @@ -119,7 +119,7 @@ json_t *fence_get_json_names (fence_t *f) return f->names; } -int fence_add_request_data (fence_t *f, json_t *ops) +int fence_add_request_ops (fence_t *f, json_t *ops) { json_t *op; int i; diff --git a/src/modules/kvs/fence.h b/src/modules/kvs/fence.h index eab9f6b2f980..14ed60f8d4d0 100644 --- a/src/modules/kvs/fence.h +++ b/src/modules/kvs/fence.h @@ -22,10 +22,10 @@ json_t *fence_get_json_ops (fence_t *f); json_t *fence_get_json_names (fence_t *f); -/* fence_add_request_data() should be called with data on each +/* fence_add_request_ops() should be called with ops on each * request, even if ops is NULL */ -int fence_add_request_data (fence_t *f, json_t *ops); +int fence_add_request_ops (fence_t *f, json_t *ops); /* copy the request message into the fence, where it can be retrieved * later. diff --git a/src/modules/kvs/kvs.c b/src/modules/kvs/kvs.c index 13df53a55d10..49d315384af8 100644 --- a/src/modules/kvs/kvs.c +++ b/src/modules/kvs/kvs.c @@ -1734,8 +1734,8 @@ static void relaycommit_request_cb (flux_t *h, flux_msg_handler_t *mh, goto error; } - if (fence_add_request_data (f, ops) < 0) { - flux_log_error (h, "%s: fence_add_request_data", __FUNCTION__); + if (fence_add_request_ops (f, ops) < 0) { + flux_log_error (h, "%s: fence_add_request_ops", __FUNCTION__); goto error; } @@ -1809,8 +1809,8 @@ static void commit_request_cb (flux_t *h, flux_msg_handler_t *mh, if (fence_add_request_copy (f, msg) < 0) goto error; if (ctx->rank == 0) { - if (fence_add_request_data (f, ops) < 0) { - flux_log_error (h, "%s: fence_add_request_data", __FUNCTION__); + if (fence_add_request_ops (f, ops) < 0) { + flux_log_error (h, "%s: fence_add_request_ops", __FUNCTION__); goto error; } diff --git a/src/modules/kvs/test/commit.c b/src/modules/kvs/test/commit.c index ebff06146379..247432286500 100644 --- a/src/modules/kvs/test/commit.c +++ b/src/modules/kvs/test/commit.c @@ -205,8 +205,8 @@ void commit_mgr_basic_tests (void) ops = json_array (); ops_append (ops, "key1", "1", 0); - ok (fence_add_request_data (f, ops) == 0, - "fence_add_request_data add works"); + ok (fence_add_request_ops (f, ops) == 0, + "fence_add_request_ops add works"); json_decref (ops); @@ -275,8 +275,8 @@ void create_ready_commit (commit_mgr_t *cm, ops = json_array (); ops_append (ops, key, val, op_flags); - ok (fence_add_request_data (f, ops) == 0, - "fence_add_request_data add works"); + ok (fence_add_request_ops (f, ops) == 0, + "fence_add_request_ops add works"); json_decref (ops); @@ -870,8 +870,8 @@ void commit_basic_iter_not_ready_tests (void) ops = json_array (); ops_append (ops, "key1", "1", 0); - ok (fence_add_request_data (f1, ops) == 0, - "fence_add_request_data add works"); + ok (fence_add_request_ops (f1, ops) == 0, + "fence_add_request_ops add works"); json_decref (ops); @@ -1367,8 +1367,8 @@ void commit_process_malformed_operation (void) ok ((f = fence_create ("malformed", 1, 0)) != NULL, "fence_create works"); - ok (fence_add_request_data (f, ops) == 0, - "fence_add_request_data add works"); + ok (fence_add_request_ops (f, ops) == 0, + "fence_add_request_ops add works"); /* Submit fence_t to commit_mgr */ diff --git a/src/modules/kvs/test/fence.c b/src/modules/kvs/test/fence.c index 3559f96fe641..a5acc7553d6d 100644 --- a/src/modules/kvs/test/fence.c +++ b/src/modules/kvs/test/fence.c @@ -62,8 +62,8 @@ void basic_api_tests (void) ops = json_array (); json_array_append_new (ops, json_string ("A")); - ok (fence_add_request_data (f, ops) == 0, - "initial fence_add_request_data add works"); + ok (fence_add_request_ops (f, ops) == 0, + "initial fence_add_request_ops add works"); ok ((o = fence_get_json_ops (f)) != NULL, "initial fence_get_json_ops call works"); @@ -71,9 +71,9 @@ void basic_api_tests (void) ok (json_equal (ops, o) == true, "initial fence_get_json_ops match"); - ok (fence_add_request_data (f, ops) < 0 + ok (fence_add_request_ops (f, ops) < 0 && errno == EOVERFLOW, - "fence_add_request_data fails with EOVERFLOW when exceeding nprocs"); + "fence_add_request_ops fails with EOVERFLOW when exceeding nprocs"); json_decref (ops); @@ -123,8 +123,8 @@ void ops_tests (void) ok (fence_count_reached (f) == false, "initial fence_count_reached() is false"); - ok (fence_add_request_data (f, NULL) == 0, - "fence_add_request_data works with NULL ops"); + ok (fence_add_request_ops (f, NULL) == 0, + "fence_add_request_ops works with NULL ops"); ok (fence_count_reached (f) == false, "fence_count_reached() is still false"); @@ -133,8 +133,8 @@ void ops_tests (void) ops = json_array (); json_array_append_new (ops, json_string ("A")); - ok (fence_add_request_data (f, ops) == 0, - "fence_add_request_data add works"); + ok (fence_add_request_ops (f, ops) == 0, + "fence_add_request_ops add works"); json_decref (ops); @@ -145,8 +145,8 @@ void ops_tests (void) ops = json_array (); json_array_append_new (ops, json_string ("B")); - ok (fence_add_request_data (f, ops) == 0, - "fence_add_request_data add works"); + ok (fence_add_request_ops (f, ops) == 0, + "fence_add_request_ops add works"); json_decref (ops); @@ -222,8 +222,8 @@ fence_t *create_fence (const char *name, const char *opname, int flags) ops = json_array (); json_array_append_new (ops, json_string (opname)); - ok (fence_add_request_data (f, ops) == 0, - "fence_add_request_data add works"); + ok (fence_add_request_ops (f, ops) == 0, + "fence_add_request_ops add works"); json_decref (ops); diff --git a/src/modules/kvs/test/kvsroot.c b/src/modules/kvs/test/kvsroot.c index b97ce6601663..8a47a8b62acb 100644 --- a/src/modules/kvs/test/kvsroot.c +++ b/src/modules/kvs/test/kvsroot.c @@ -180,7 +180,7 @@ void basic_commit_mgr_tests (void) /* not a real operation */ json_array_append_new (ops, json_string ("foo")); - fence_add_request_data (f, ops); + fence_add_request_ops (f, ops); json_decref (ops); From 5c77af8aa1efa5a449a7af96e532d218b25b7440 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 14 Feb 2018 14:40:52 -0800 Subject: [PATCH 3/5] module/kvs: Refactor commit_process() Remove unnecessary branch check, as ops array is always non-NULL (but can be an empty array). --- src/modules/kvs/commit.c | 84 ++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 43 deletions(-) diff --git a/src/modules/kvs/commit.c b/src/modules/kvs/commit.c index a5f823d5a291..4cc710fafc35 100644 --- a/src/modules/kvs/commit.c +++ b/src/modules/kvs/commit.c @@ -739,55 +739,53 @@ commit_process_t commit_process (commit_t *c, * the commit to be self-contained in the rootcpy until it * is unrolled later on. */ - if (fence_get_json_ops (c->f)) { - json_t *op, *dirent; - const char *missing_ref = NULL; - json_t *ops = fence_get_json_ops (c->f); - int i, len = json_array_size (ops); - const char *key; - int flags; - - /* Caller didn't call commit_iter_missing_refs() */ - if (zlist_first (c->missing_refs_list)) - goto stall_load; - - for (i = 0; i < len; i++) { - missing_ref = NULL; - op = json_array_get (ops, i); - assert (op != NULL); - if (txn_decode_op (op, &key, &flags, &dirent) < 0) { - c->errnum = errno; - break; - } - if (commit_link_dirent (c, - current_epoch, - c->rootcpy, - key, - dirent, - flags, - &missing_ref) < 0) { - c->errnum = errno; + json_t *op, *dirent; + const char *missing_ref = NULL; + json_t *ops = fence_get_json_ops (c->f); + int i, len = json_array_size (ops); + const char *key; + int flags; + + /* Caller didn't call commit_iter_missing_refs() */ + if (zlist_first (c->missing_refs_list)) + goto stall_load; + + for (i = 0; i < len; i++) { + missing_ref = NULL; + op = json_array_get (ops, i); + assert (op != NULL); + if (txn_decode_op (op, &key, &flags, &dirent) < 0) { + c->errnum = errno; + break; + } + if (commit_link_dirent (c, + current_epoch, + c->rootcpy, + key, + dirent, + flags, + &missing_ref) < 0) { + c->errnum = errno; + break; + } + if (missing_ref) { + if (zlist_push (c->missing_refs_list, + (void *)missing_ref) < 0) { + c->errnum = ENOMEM; break; } - if (missing_ref) { - if (zlist_push (c->missing_refs_list, - (void *)missing_ref) < 0) { - c->errnum = ENOMEM; - break; - } - } } + } - if (c->errnum != 0) { - /* empty missing_refs_list to prevent mistakes later */ - while ((missing_ref = zlist_pop (c->missing_refs_list))); - return COMMIT_PROCESS_ERROR; - } + if (c->errnum != 0) { + /* empty missing_refs_list to prevent mistakes later */ + while ((missing_ref = zlist_pop (c->missing_refs_list))); + return COMMIT_PROCESS_ERROR; + } - if (zlist_first (c->missing_refs_list)) - goto stall_load; + if (zlist_first (c->missing_refs_list)) + goto stall_load; - } c->state = COMMIT_STATE_STORE; /* fallthrough */ } From c101d1d091f4065792d5e7beb9375dc822ed88b0 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 14 Feb 2018 16:00:47 -0800 Subject: [PATCH 4/5] common/libkvs: Check inputs in flux_kvs_fence Check that name is not NULL and nprocs is > 0. --- src/common/libkvs/kvs_commit.c | 5 +++++ src/common/libkvs/test/kvs_commit.c | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/common/libkvs/kvs_commit.c b/src/common/libkvs/kvs_commit.c index b14bb2eb7b6e..936556702f89 100644 --- a/src/common/libkvs/kvs_commit.c +++ b/src/common/libkvs/kvs_commit.c @@ -37,6 +37,11 @@ flux_future_t *flux_kvs_fence (flux_t *h, int flags, const char *name, { const char *namespace; + if (!name || nprocs <= 0) { + errno = EINVAL; + return NULL; + } + if (!(namespace = flux_kvs_get_namespace (h))) return NULL; diff --git a/src/common/libkvs/test/kvs_commit.c b/src/common/libkvs/test/kvs_commit.c index 675ef6f5fb8f..1285e5d8ae5d 100644 --- a/src/common/libkvs/test/kvs_commit.c +++ b/src/common/libkvs/test/kvs_commit.c @@ -15,7 +15,11 @@ void errors (void) errno = 0; ok (flux_kvs_fence (NULL, 0, NULL, 0, NULL) == NULL && errno == EINVAL, - "flux_kvs_fence fails on bad input"); + "flux_kvs_fence fails on bad params"); + + errno = 0; + ok (flux_kvs_fence (NULL, 0, "foo", 1, NULL) == NULL && errno == EINVAL, + "flux_kvs_fence fails on bad handle"); errno = 0; ok (flux_kvs_commit (NULL, 0, NULL) == NULL && errno == EINVAL, From 134370e97d4cc16f1e4cd519f0ebc0078ff48959 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Wed, 14 Feb 2018 16:04:28 -0800 Subject: [PATCH 5/5] modules/kvs: Add fence_create() input check Check that name is not NULL and nprocs is > 0. --- src/modules/kvs/fence.c | 24 +++++++++++++----------- src/modules/kvs/test/fence.c | 3 +++ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/modules/kvs/fence.c b/src/modules/kvs/fence.c index dc6a3852a47c..833ccbfa7a42 100644 --- a/src/modules/kvs/fence.c +++ b/src/modules/kvs/fence.c @@ -60,10 +60,14 @@ void fence_destroy (fence_t *f) fence_t *fence_create (const char *name, int nprocs, int flags) { - fence_t *f; + fence_t *f = NULL; json_t *s = NULL; int saved_errno; + if (!name || nprocs <= 0) { + saved_errno = EINVAL; + goto error; + } if (!(f = calloc (1, sizeof (*f))) || !(f->ops = json_array ()) || !(f->names = json_array ()) @@ -74,16 +78,14 @@ fence_t *fence_create (const char *name, int nprocs, int flags) f->nprocs = nprocs; f->flags = flags; f->aux_int = 0; - if (name) { - if (!(s = json_string (name))) { - saved_errno = ENOMEM; - goto error; - } - if (json_array_append_new (f->names, s) < 0) { - json_decref (s); - saved_errno = ENOMEM; - goto error; - } + if (!(s = json_string (name))) { + saved_errno = ENOMEM; + goto error; + } + if (json_array_append_new (f->names, s) < 0) { + json_decref (s); + saved_errno = ENOMEM; + goto error; } return f; diff --git a/src/modules/kvs/test/fence.c b/src/modules/kvs/test/fence.c index a5acc7553d6d..edf02a05eb69 100644 --- a/src/modules/kvs/test/fence.c +++ b/src/modules/kvs/test/fence.c @@ -35,6 +35,9 @@ void basic_api_tests (void) flux_msg_t *request; int count = 0; + ok (fence_create (NULL, 0, 0) == NULL, + "fence_create fails on bad input"); + ok ((f = fence_create ("foo", 1, 3)) != NULL, "fence_create works");