common/libkvs: Refactor treeobj_copy_dir()

Change to treeobj_copy() and make function a shallow copy for
all treeobj types.

Update unit tests appropriately.

Update callers appropriately.

rebase later : put earlier  in commits

src/common/libkvs/test/treeobj.c

@@ -338,91 +338,147 @@ void test_dir (void)
     json_decref (dir);
 }
 
-void test_copy_dir (void)
+void test_copy (void)
 {
-    json_t *dir, *dircpy;
-    json_t *val1, *val2, *val3;
-    json_t *test;
-
-    /* create couple test values */
-    val1 = treeobj_create_val ("a", 1);
-    val2 = treeobj_create_val ("b", 1);
-    val3 = treeobj_create_val ("c", 1);
-    if (!val1 || !val2 || !val3)
-        BAIL_OUT ("can't continue without test values");
+    json_t *val, *symlink, *dirref, *valref, *dir;
+    json_t *valcpy, *symlinkcpy, *dirrefcpy, *valrefcpy, *dircpy;
+    json_t *val1, *val2;
 
     /* First, some corner case tests */
 
-    ok (treeobj_copy_dir (val1) == NULL,
-        "tree_copy_dir fails on non-dir");
+    ok (treeobj_copy (NULL) == NULL,
+        "tree_copy fails on bad input");
 
-    /* Next, test to show json_copy() is not safe.
-     *
-     * json_copy(), which makes a shallow copy of a JSON object,
-     * cannot be used directly on a RFC 11 dir object, since the
-     * directory itself is one level down under the "data" key.
-    */
+    /* Test val copy */
+
+    val = treeobj_create_val ("a", 1);
+    if (!val)
+        BAIL_OUT ("can't continue without test val");
+
+    ok ((valcpy = treeobj_copy (val)) != NULL,
+        "treeobj_copy worked on val");
+
+    ok (val != valcpy && json_equal (val, valcpy) == 1,
+        "treeobj_copy returned duplicate val copy");
+
+    json_decref (val);
+    json_decref (valcpy);
+
+    /* Test symlink copy */
+
+    symlink = treeobj_create_symlink ("abcdefgh");
+    if (!symlink)
+        BAIL_OUT ("can't continue without test symlink");
+
+    ok ((symlinkcpy = treeobj_copy (symlink)) != NULL,
+        "treeobj_copy worked on symlink");
+
+    ok (symlink != symlinkcpy && json_equal (symlink, symlinkcpy) == 1,
+        "treeobj_copy returned duplicate symlink copy");
+
+    json_decref (symlink);
+    json_decref (symlinkcpy);
+
+    /* Test dirref copy */
+
+    dirref = treeobj_create_dirref (blobrefs[0]);
+    if (!dirref)
+        BAIL_OUT ("can't continue without test dirref");
+
+    ok ((dirrefcpy = treeobj_copy (dirref)) != NULL,
+        "treeobj_copy worked on dirref");
+
+    ok (dirref != dirrefcpy && json_equal (dirref, dirrefcpy) == 1,
+        "treeobj_copy returned duplicate dirref copy");
+
+    ok (treeobj_append_blobref (dirref, blobrefs[1]) == 0,
+        "treeobj_append_blobref success");
+
+    ok (json_equal (dirref, dirrefcpy) == 0,
+        "change to one dirref did not affect other");
+
+    json_decref (dirref);
+    json_decref (dirrefcpy);
+
+    /* Test valref copy */
+
+    valref = treeobj_create_valref (blobrefs[0]);
+    if (!valref)
+        BAIL_OUT ("can't continue without test valref");
+
+    ok ((valrefcpy = treeobj_copy (valref)) != NULL,
+        "treeobj_copy worked on valref");
+
+    ok (valref != valrefcpy && json_equal (valref, valrefcpy) == 1,
+        "treeobj_copy returned duplicate valref copy");
+
+    ok (treeobj_append_blobref (valref, blobrefs[1]) == 0,
+        "treeobj_append_blobref success");
+
+    ok (json_equal (valref, valrefcpy) == 0,
+        "change to one valref did not affect other");
+
+    json_decref (valref);
+    json_decref (valrefcpy);
+
+    /* Test dir copy */
+
+    dir = treeobj_create_dir ();
+    val1 = treeobj_create_val ("a", 1);
+    val2 = treeobj_create_val ("b", 1);
+    if (!dir || !val1 || !val2)
+        BAIL_OUT ("can't continue without test dir");
 
-    ok ((dir = treeobj_create_dir ()) != NULL,
-        "treeobj_create_dir works");
     ok (treeobj_insert_entry (dir, "a", val1) == 0,
         "treeobj_insert_entry works");
-    ok (treeobj_insert_entry (dir, "b", val2) == 0,
-        "treeobj_insert_entry works");
-    ok (treeobj_get_count (dir) == 2,
-        "treeobj_get_count gets correct count");
 
-    ok ((dircpy = json_copy (dir)) != NULL,
-        "json_copy works");
+    ok ((dircpy = treeobj_copy (dir)) != NULL,
+        "treeobj_copy worked on dir");
 
-    /* change "b" to "c" in original */
-    ok (treeobj_insert_entry (dir, "b", val3) == 0,
-        "treeobj_insert_entry works");
+    ok (dir != dircpy && json_equal (dir, dircpy) == 1,
+        "treeobj_copy returned duplicate dir copy");
 
-    ok (json_equal (dir, dircpy) == 1,
-        "dir and dircpy are equal, json_copy() not safe");
+    /* change "a" to "b" in main dir */
+    ok (treeobj_insert_entry (dir, "a", val2) == 0,
+        "treeobj_insert_entry success");
+
+    ok (json_equal (dir, dircpy) == 0,
+        "change to one dir did not affect other");
 
     json_decref (dir);
     json_decref (dircpy);
+    json_decref (val1);
+    json_decref (val2);
 
-    /* Now test to show treeobj_copy_dir() is safe */
+    /* Show that json copy is not safe compared to treeobj_copy()
+     * above */
+
+    dir = treeobj_create_dir ();
+    val1 = treeobj_create_val ("a", 1);
+    val2 = treeobj_create_val ("b", 1);
+    if (!dir || !val1 || !val2)
+        BAIL_OUT ("can't continue without test dir");
 
-    ok ((dir = treeobj_create_dir ()) != NULL,
-        "treeobj_create_dir works");
     ok (treeobj_insert_entry (dir, "a", val1) == 0,
         "treeobj_insert_entry works");
-    ok (treeobj_insert_entry (dir, "b", val2) == 0,
-        "treeobj_insert_entry works");
-    ok (treeobj_get_count (dir) == 2,
-        "treeobj_get_count gets correct count");
 
-    ok ((dircpy = treeobj_copy_dir (dir)) != NULL,
-        "treeobj_copy_dir works");
-
-    /* change "b" to "c" in original */
-    ok (treeobj_insert_entry (dir, "b", val3) == 0,
-        "treeobj_insert_entry works");
+    ok ((dircpy = json_copy (dir)) != NULL,
+        "json_copy worked on dir");
 
-    ok (json_equal (dir, dircpy) == 0,
-        "dir and dircpy are not equal, treeobj_copy_dir() safe");
+    ok (dir != dircpy && json_equal (dir, dircpy) == 1,
+        "json_copy returned duplicate dir copy");
 
-    /* Make sure values are in the right copies */
-    ok ((test = treeobj_get_entry (dir, "b")) != NULL,
-        "treeobj_get_entry works");
-    ok (json_equal (test, val3) == 1,
-        "correct value is in original");
+    /* change "a" to "b" in main dir */
+    ok (treeobj_insert_entry (dir, "a", val2) == 0,
+        "treeobj_insert_entry success");
 
-    ok ((test = treeobj_get_entry (dircpy, "b")) != NULL,
-        "treeobj_get_entry works");
-    ok (json_equal (test, val2) == 1,
-        "correct value is in copy");
+    ok (json_equal (dir, dircpy) == 1,
+        "change to one dir did affect other");
 
     json_decref (dir);
     json_decref (dircpy);
-
     json_decref (val1);
     json_decref (val2);
-    json_decref (val3);
 }
 
 void test_deep_copy (void)
@@ -550,7 +606,7 @@ void test_deep_copy (void)
     json_decref (val2);
     json_decref (val3);
 
-    /* Test dir copy compared to copy_dir function */
+    /* Test dir copy compared to copy function */
 
     dir = treeobj_create_dir ();
     subdir = treeobj_create_dir();
@@ -567,11 +623,11 @@ void test_deep_copy (void)
     ok (treeobj_insert_entry (dir, "subdir", subdir) == 0,
         "treeobj_insert_entry works");
 
-    ok ((dircpy = treeobj_copy_dir (dir)) != NULL,
-        "treeobj_copy_dir worked on dir");
+    ok ((dircpy = treeobj_copy (dir)) != NULL,
+        "treeobj_copy worked on dir");
 
     ok (dir != dircpy && json_equal (dir, dircpy) == 1,
-        "treeobj_copy_dir returned duplicate dir copy");
+        "treeobj_copy returned duplicate dir copy");
 
     ok ((subdir1 = treeobj_get_entry (dir, "subdir")) != NULL,
         "treeobj_get_entry got subdir");
@@ -583,7 +639,7 @@ void test_deep_copy (void)
         "treeobj_insert_entry success");
 
     ok (json_equal (dir, dircpy) == 1,
-        "change to one dir *did* affect other, b/c treeobj_copy_dir does only a 1 level copy");
+        "change to one dir *did* affect other, b/c treeobj_copy does only a 1 level copy");
 
     json_decref (dir);
     json_decref (dircpy);
@@ -708,7 +764,7 @@ int main(int argc, char** argv)
     test_val ();
     test_dirref ();
     test_dir ();
-    test_copy_dir ();
+    test_copy ();
     test_deep_copy ();
     test_symlink ();
     test_corner_cases ();

src/common/libkvs/treeobj.c

@@ -263,35 +263,41 @@ int treeobj_insert_entry (json_t *obj, const char *name, json_t *obj2)
     return 0;
 }
 
-json_t *treeobj_deep_copy (json_t *obj)
+json_t *treeobj_copy (json_t *obj)
 {
-    json_t *cpy;
-
-    if (treeobj_validate (obj) < 0) {
-        errno = EINVAL;
-        return NULL;
-    }
-
-    if (!(cpy = json_deep_copy (obj)))
-        return NULL;
-    return cpy;
-}
-
-json_t *treeobj_copy_dir (json_t *obj)
-{
-    const char *type;
     json_t *data;
     json_t *cpy;
     json_t *datacpy;
     int save_errno;
 
-    if (treeobj_unpack (obj, &type, &data) < 0
-            || strcmp (type, "dir") != 0) {
+    if (treeobj_unpack (obj, NULL, &data) < 0
+        || treeobj_validate (obj) < 0) {
         errno = EINVAL;
         return NULL;
     }
-    if (!(cpy = treeobj_create_dir ()))
-        return NULL;
+    if (treeobj_is_val (obj)) {
+        if (!(cpy = treeobj_create_val (NULL, 0)))
+            return NULL;
+    }
+    else if (treeobj_is_symlink (obj)) {
+        /* treeobj_create_symlink() fails on NULL input, use empty
+         * string.
+         */
+        if (!(cpy = treeobj_create_symlink ("")))
+            return NULL;
+    }
+    else if (treeobj_is_valref (obj)) {
+        if (!(cpy = treeobj_create_valref (NULL)))
+            return NULL;
+    }
+    else if (treeobj_is_dirref (obj)) {
+        if (!(cpy = treeobj_create_dirref (NULL)))
+            return NULL;
+    }
+    else { /* treeobj_is_dir (obj) */
+        if (!(cpy = treeobj_create_dir ()))
+            return NULL;
+    }
     if (!(datacpy = json_copy (data))) {
         save_errno = errno;
         json_decref (cpy);
@@ -308,6 +314,20 @@ json_t *treeobj_copy_dir (json_t *obj)
     return cpy;
 }
 
+json_t *treeobj_deep_copy (json_t *obj)
+{
+    json_t *cpy;
+
+    if (treeobj_validate (obj) < 0) {
+        errno = EINVAL;
+        return NULL;
+    }
+
+    if (!(cpy = json_deep_copy (obj)))
+        return NULL;
+    return cpy;
+}
+
 int treeobj_append_blobref (json_t *obj, const char *blobref)
 {
     const char *type;

src/common/libkvs/treeobj.h

@@ -69,16 +69,16 @@ json_t *treeobj_get_entry (json_t *obj, const char *name);
 int treeobj_insert_entry (json_t *obj, const char *name, json_t *obj2);
 int treeobj_delete_entry (json_t *obj, const char *name);
 
+/* Shallow copy a treeobj
+ * Note that this is not a shallow copy on the json object, but is a
+ * shallow copy on the data within a tree object.  For example, for a
+ * dir object, the first level of directory entries will be copied.
+ */
+json_t *treeobj_copy (json_t *obj);
+
 /* Deep copy a treeobj */
 json_t *treeobj_deep_copy (json_t *obj);
 
-/* Copy a treeobj dir.
- * This function performs a shallow copy on a dir object, copying the
- * first level of directory entries.  Use this function over json_copy()
- * on dir objects, as the latter is not safe on treeobj objects.
- */
-json_t *treeobj_copy_dir (json_t *obj);
-
 /* add blobref to dirref,valref object.
  * Return 0 on success, -1 on failure with errno set.
  */

src/modules/kvs/commit.c

@@ -585,7 +585,7 @@ static int commit_link_dirent (commit_t *c, int current_epoch,
             }
 
             /* do not corrupt store by modifying orig. */
-            if (!(subdir = treeobj_copy_dir (subdir))) {
+            if (!(subdir = treeobj_copy (subdir))) {
                 saved_errno = errno;
                 goto done;
             }
@@ -712,7 +712,7 @@ commit_process_t commit_process (commit_t *c,
                 goto stall_load;
             }
 
-            if (!(c->rootcpy = treeobj_copy_dir (rootdir))) {
+            if (!(c->rootcpy = treeobj_copy (rootdir))) {
                 c->errnum = errno;
                 return COMMIT_PROCESS_ERROR;
             }