Merge pull request #7186 from chu11/flux_dump_fsck_cleanup

flux-dump/flux-fsck misc cleanup

Author: mergify[bot]

src/cmd/builtin/dump.c

@@ -169,8 +169,11 @@ static void dump_valref (struct archive *ar,
     const void *data;
     size_t len;
 
-    /* We need the total size before we start writing archive data,
-     * so make a first pass, saving the data for writing later.
+    /* Load all data comprising the valref before starting the archive
+     * entry. This is because the total size of the value must
+     * calculated and written before any data, and any load errors
+     * must be detected so the whole entry can be skipped if
+     * --ignore-failed-read.
      */
     /* N.B. first attempt was to save the futures in an array, but ran into:
      *   flux: ev_epoll.c:134: epoll_modify: Assertion `("libev: I/O watcher
@@ -188,7 +191,7 @@ static void dump_valref (struct archive *ar,
                                           treeobj_get_blobref (treeobj, i),
                                           content_flags))
             || flux_future_get (f, (const void **)&msg) < 0
-            || flux_response_decode_raw (msg, NULL, &data, &len) < 0) {
+            || flux_response_decode_raw (msg, NULL, NULL, &len) < 0) {
             read_error ("%s: missing blobref %d: %s",
                         path,
                         i,

src/cmd/builtin/fsck.c

@@ -25,6 +25,7 @@
 #include "src/common/libutil/blobref.h"
 #include "src/common/libcontent/content.h"
 #include "src/common/libczmqcontainers/czmq_containers.h"
+#include "ccan/ptrint/ptrint.h"
 #include "ccan/str/str.h"
 
 #include "builtin.h"
@@ -121,24 +122,24 @@ static void valref_validate_continuation (flux_future_t *f, void *arg)
     struct fsck_valref_data *fvd = arg;
 
     if (flux_rpc_get (f, NULL) < 0) {
-        int *index = flux_future_aux_get (f, "index");
+        int index = ptr2int (flux_future_aux_get (f, "index"));
         if (fvd->ctx->verbose) {
             if (errno == ENOENT)
                 errmsg (fvd->ctx,
                         "%s: missing blobref index=%d",
                         fvd->path,
-                        (*index));
+                        index);
             else
                 errmsg (fvd->ctx,
                         "%s: error retrieving blobref index=%d: %s",
                         fvd->path,
-                        (*index),
+                        index,
                         future_strerror (f, errno));
         }
         fvd->errorcount++;
         fvd->errnum = errno;     /* we'll report the last errno */
         if (fvd->ctx->repair && errno == ENOENT)
-            save_missing_ref_index (fvd, *index);
+            save_missing_ref_index (fvd, index);
     }
     fvd->in_flight--;
 
@@ -159,7 +160,6 @@ static void valref_validate (struct fsck_valref_data *fvd)
     ssize_t hash_size;
     const char *blobref;
     flux_future_t *f;
-    int *indexp;
 
     blobref = treeobj_get_blobref (fvd->treeobj, fvd->index);
 
@@ -170,10 +170,7 @@ static void valref_validate (struct fsck_valref_data *fvd)
         log_err_exit ("failed to validate valref blob");
     if (flux_future_then (f, -1, valref_validate_continuation, fvd) < 0)
         log_err_exit ("cannot validate valref blob");
-    if (!(indexp = (int *)malloc (sizeof (int))))
-        log_err_exit ("cannot allocate index memory");
-    (*indexp) = fvd->index;
-    if (flux_future_aux_set (f, "index", indexp, free) < 0)
+    if (flux_future_aux_set (f, "index", int2ptr (fvd->index), NULL) < 0)
         log_err_exit ("could not save index value");
 }
 
@@ -453,8 +450,7 @@ static void fsck_dirref (struct fsck_ctx *ctx,
             warn (ctx, "%s unlinked due to missing blobref", path);
             ctx->unlink_dir_count++;
         }
-        flux_future_destroy (f);
-        return;
+        goto cleanup;
     }
     if (!(treeobj_deref = treeobj_decodeb (buf, buflen))) {
         errmsg (ctx, "%s: could not decode directory", path);

t/t2807-dump-cmd.t

@@ -49,7 +49,8 @@ test_expect_success 'load kvs and create some kvs content' '
 	flux kvs put --no-merge x=$(cat x.val) &&
 	flux kvs link --target-namespace=smurf otherthing z &&
 	flux kvs put --no-merge w= &&
-	flux kvs put --no-merge --append w=foo
+	flux kvs put --no-merge --append w=foo &&
+	flux kvs put --no-merge --append w=bar
 '
 test_expect_success 'unload kvs' '
 	flux module remove kvs
@@ -81,16 +82,16 @@ test_expect_success 'unload content-sqlite' '
 # (1) rootdir 5th version (z is contained in root)
 # (1) rootdir 6th version (w probably contained in root)
 # (3) rootdir 7th version ('w' empty blobref + append to 'w')
-# Total: 12 blobs
+# (2) rootdir 8th version (append to 'w')
+# Total: 14 blobs
 #
 test_expect_success 'count blobs representing those five keys' '
-	echo 12 >blobcount.exp &&
+	echo 14 >blobcount.exp &&
 	countblobs >blobcount.out &&
 	test_cmp blobcount.exp blobcount.out
 '
-test_expect_success 'remove backing file and load content-sqlite' '
-	rm -f content.sqlite &&
-	flux module load content-sqlite
+test_expect_success 'load content-sqlite and truncate old file to start fresh' '
+	flux module load content-sqlite truncate
 '
 test_expect_success 'restore content' '
 	flux restore --checkpoint foo.tar
@@ -111,7 +112,8 @@ test_expect_success 'unload content-sqlite' '
 '
 
 # Intermediate rootdir versions are not preserved across dump/restore.
-# Expect a blobcount of 4: rootdir 5th version + 'a' + 'b' + 'x',
+# Expect a blobcount of 4: rootdir 8th version + 'a' + 'b' + 'x',
+# N.B. 'w' is now contained in the root
 #
 test_expect_success 'count blobs after restore'\'s' implicit garbage collection' '
 	echo 4 >blobcount2.exp &&
@@ -129,7 +131,7 @@ test_expect_success 'verify that exact KVS content was restored' '
 	test $(flux kvs get x) = $(cat x.val) &&
 	test $(flux kvs readlink y) = "linkedthing" &&
 	test $(flux kvs readlink z) = "smurf::otherthing" &&
-	test $(flux kvs get w) = "foo"
+	test $(flux kvs get w) = "foobar"
 '
 test_expect_success 'now restore to key and verify content' '
 	flux restore -v --key zz foo.tar &&
@@ -255,6 +257,44 @@ test_expect_success UTF8_LOCALE 'restore UTF-8 dump' '
 	    flux kvs get ƒuzzybunny)
 '
 
+# Cover value with a very large number of appends
+
+test_expect_success LONGTEST 'load content-sqlite and truncate old file to start fresh' '
+	flux module load content-sqlite truncate
+'
+# N.B. from 1000 to 3000 instead of 0 to 2000, easier to debug errors
+# using fold(1) (i.e. all numbers same width)
+test_expect_success LONGTEST 'load kvs and create some kvs content' '
+	flux module load kvs &&
+	for i in `seq 1000 3000`; do
+	    flux kvs put --append bigval=${i}
+	done &&
+	flux kvs get bigval > bigval.exp
+'
+test_expect_success LONGTEST 'unload kvs' '
+	flux module remove kvs
+'
+test_expect_success LONGTEST 'dump default=kvs-primary checkpoint' '
+	flux dump --checkpoint bigval.tar
+'
+test_expect_success LONGTEST 'reload content-sqlite, truncate old file to start fresh' '
+	flux module reload content-sqlite truncate
+'
+test_expect_success LONGTEST 'restore content' '
+	flux restore --checkpoint bigval.tar
+'
+test_expect_success LONGTEST 'load kvs and check bigval value' '
+	flux module load kvs &&
+	flux kvs get bigval > bigval.out &&
+	test_cmp bigval.out bigval.exp
+'
+test_expect_success LONGTEST 'unload kvs' '
+	flux module remove kvs
+'
+test_expect_success LONGTEST 'unload content-sqlite' '
+	flux module remove content-sqlite
+'
+
 # Cover --size-limit
 
 test_expect_success 'create bigdump.tar with a 12M blob in it' '