addressing review

Author: marco6

.github/scripts/generate-snapshot.sh

@@ -17,7 +17,7 @@ USAGE
 	exit 0
 }
 
-# Parse arguments
+# Parse arguments.
 while [ $# -gt 0 ]; do
 	case "$1" in
         --main-only)
@@ -59,23 +59,23 @@ if [ -z "$OUTPUT_DIR" ]; then
 	usage
 fi
 
-# Cleanup
+# Cleanup.
 trap 'rm -f temp temp-wal temp-shm' EXIT
 
 SQLITE3_CHECKPOINT_ON_CLOSE=""
 if [ "$MAIN_ONLY" -eq 0 ]; then
     SQLITE3_CHECKPOINT_ON_CLOSE=".dbconfig no_ckpt_on_close on"
 fi
 
-# First generate a sqlite3 database
+# First generate a sqlite3 database.
 echo -n "Generating database with $ROW_COUNT rows..."
 cat <<EOF | sqlite3 temp > /dev/null 2>&1
 PRAGMA journal_mode=WAL;
 PRAGMA wal_autocheckpoint=$WAL_AUTOCHECKPOINT;
 $SQLITE3_CHECKPOINT_ON_CLOSE
 
 CREATE TABLE test(id INTEGER PRIMARY KEY, value TEXT NOT NULL);
-WITH sequence AS (
+WITH RECURSIVE sequence AS (
    SELECT 1 AS id
 
    UNION ALL
@@ -87,12 +87,10 @@ WITH sequence AS (
 INSERT OR REPLACE INTO test
 SELECT id, hex(randomblob(16))
 FROM sequence;
-
 EOF
 echo "Done"
 
-
-# Now generate a dqlite snapshot from the sqlite3 database
+# Now generate a dqlite snapshot from the sqlite3 database.
 echo -n "Generating snapshot in '$OUTPUT_DIR'..."
 cat <<EOF | dqlite-utils > /dev/null 2>&1
 .snapshot

src/fsm.c

@@ -202,12 +202,12 @@ static int decodeDatabase(const struct registry *r,
 	const size_t page_size = r->config->vfs.page_size;
 	dqlite_assert((header.main_size % page_size) == 0);
 
-	size_t page_count = (size_t)header.main_size / page_size;
-	void **pages = raft_malloc(sizeof(void *) * page_count);
+	size_t main_page_count = (size_t)header.main_size / page_size;
+	void **pages = raft_malloc(sizeof(void *) * main_page_count);
 	if (pages == NULL) {
 		return RAFT_NOMEM;
 	}
-	for (size_t i = 0; i < page_count; i++) {
+	for (size_t i = 0; i < main_page_count; i++) {
 		pages[i] = (void *)(cursor->p + i * page_size);
 	}
 	cursor->p += header.main_size;
@@ -225,15 +225,18 @@ static int decodeDatabase(const struct registry *r,
 		const unsigned n_frames =
 		    (unsigned)((header.wal_size - (size_t)wal_header_size) /
 			       wal_frame_size);
-		const char *wal = cursor->p;
-		const void *last_frame =
-		    (const uint8_t *)wal + wal_header_size +
+		const uint8_t *wal = (uint8_t*)cursor->p;
+		// In dqlite, WAL is always well-formed (id doesn't contain
+		// partial transactions). As such, the last frame is always a
+		// commit frame and must contain the size of the database after
+		// the transaction.
+		const uint8_t *last_frame =
+		    wal + wal_header_size +
 		    n_frames * wal_frame_size - wal_frame_size;
-
 		const size_t wal_page_count = ByteGetBe32(last_frame + 4);
 		dqlite_assert(wal_page_count != 0);
 
-		if (wal_page_count > page_count) {
+		if (wal_page_count > main_page_count) {
 			void *wal_pages = raft_realloc(
 			    pages, sizeof(void *) * wal_page_count);
 			if (wal_pages == NULL) {
@@ -244,10 +247,10 @@ static int decodeDatabase(const struct registry *r,
 		}
 
 		/* Read pages in the WAL order */
-		for (const char *frame = wal + wal_header_size;
+		for (const uint8_t *frame = wal + wal_header_size;
 		     frame < wal + header.wal_size; frame += wal_frame_size) {
 			uint32_t page_number =
-			    ByteGetBe32((const uint8_t *)frame);
+			    ByteGetBe32(frame);
 			if (page_number > wal_page_count) {
 				continue;
 			}
@@ -258,7 +261,7 @@ static int decodeDatabase(const struct registry *r,
 
 		/* Verify that if the WAL resized the database, then no page is
 		 * missing. */
-		for (size_t page_number = page_count;
+		for (size_t page_number = main_page_count;
 		     page_number <= wal_page_count; page_number++) {
 			if (pages[page_number - 1] == NULL) {
 				tracef("missing page %" PRIu64 " in wal",
@@ -267,12 +270,12 @@ static int decodeDatabase(const struct registry *r,
 				return RAFT_INVALID;
 			}
 		}
-		page_count = wal_page_count;
+		main_page_count = wal_page_count;
 	}
 	cursor->p += header.wal_size;
 
 	*snapshot = (struct vfsSnapshot){
-		.page_count = page_count,
+		.page_count = main_page_count,
 		.page_size = page_size,
 		.pages = pages,
 	};

test/integration/test_node.c

@@ -268,24 +268,13 @@ __attribute__((constructor)) static void snapshot_dirs_env_init(void)
 	}
 }
 
-// Make the sanitizer happy
+// Make the sanitizer happy.
 __attribute__((destructor)) static void snapshot_dirs_env_uninit(void)
 {
 	free(snapshot_dir_env);
 	snapshot_dir_env = NULL;
 }
 
-
-RUNNER_ARGUMENT(snapshot, "directory where to find a test snapshot") {
-	if (NEXT_ARG == NULL) {
-		printf("Error: --snapshot-dirs requires an argument\n");
-		return false;
-	}
-	SHIFT;
-	snapshot_dirs[snapshot_dirs_n++] = CURRENT_ARG;
-	return true;
-}
-
 static void *setUpExistingSnapshot(const MunitParameter params[], void *user_data)
 {
 	return setUp(params, user_data, munit_parameters_get(params, SNAPSHOT_DIR_PARAM), "127.0.0.1:9001");
@@ -303,7 +292,7 @@ TEST(node, existing_snapshot, setUpExistingSnapshot, tearDownKeepDir, 0, test_sn
 	rv = dqlite_node_start(f->node);
 	munit_assert_int(rv, ==, 0);
 
-	/* Open a bunch of clients and do a query on them */
+	/* Open a client and do a query on them */
 	struct client_proto client;
 	struct rows rows;
 

test/lib/runner.h

@@ -19,9 +19,6 @@
 extern MunitSuite _main_suites[];
 extern int _main_suites_n;
 
-extern MunitArgument _main_args[];
-extern int _main_args_n;
-
 /* Maximum number of test cases for each suite */
 #define SUITE__CAP 128
 #define TEST__CAP SUITE__CAP
@@ -93,14 +90,12 @@ extern int _main_args_n;
 	} while (0)
 
 #else
-#error "backtrace enabled, but no library to support it"
+# error "backtrace enabled, but no library to support it"
 #endif /* HAVE_EXECINFO_H */
 
 #else
 
-#define PRINT_BACKTRACE(SKIP) \
-	do {                  \
-	} while (0)
+#define PRINT_BACKTRACE(SKIP) do {} while (0)
 
 #endif /* DQLITE_ASSERT_WITH_BACKTRACE */
 
@@ -133,48 +128,15 @@ extern int _main_args_n;
 	MunitSuite _main_suites[SUITE__CAP];                                  \
 	int _main_suites_n = 0;                                               \
                                                                               \
-	MunitArgument _main_args[SUITE__CAP] = {};                            \
-	int _main_args_n = 0;                                                 \
-                                                                              \
 	int main(int argc, char *argv[MUNIT_ARRAY_PARAM(argc)])               \
 	{                                                                     \
 		signal(SIGPIPE, SIG_IGN);                                     \
 		signal(SIGABRT, print_backtrace);                             \
 		dqliteTracingMaybeEnable(true);                               \
 		MunitSuite suite = { (char *)"", NULL, _main_suites, 1, 0 };  \
-		return munit_suite_main_custom(&suite, (void *)NAME, argc,    \
-					       argv, _main_args);             \
+		return munit_suite_main(&suite, (void *)NAME, argc, argv);    \
 	}
 
-/* Declare and register a new argument. */
-#define RUNNER_ARGUMENT(NAME, HELP_STR)                                       \
-	static bool argument_##NAME##_parse(                                  \
-	    const MunitSuite *suite, void *user_data, int *arg, int argc,     \
-	    char *const argv[MUNIT_ARRAY_PARAM(argc)]);                       \
-	static void argument_##NAME##_help(const MunitArgument *argument,     \
-					   void *user_data)                   \
-	{                                                                     \
-		(void)user_data;                                              \
-		printf("--%s: %s\n", argument->name, HELP_STR);               \
-	}                                                                     \
-	__attribute__((constructor)) static void argument_##NAME##_init(void) \
-	{                                                                     \
-		int n = _main_args_n;                                         \
-		_main_args[n].name = #NAME;                                   \
-		_main_args[n].parse_argument = argument_##NAME##_parse;       \
-		_main_args[n].write_help = argument_##NAME##_help;            \
-		_main_args_n = n + 1;                                         \
-	}                                                                     \
-	static bool argument_##NAME##_parse(                                  \
-	    MUNIT_UNUSED const MunitSuite *suite,                             \
-	    MUNIT_UNUSED void *user_data, MUNIT_UNUSED int *arg,              \
-	    MUNIT_UNUSED int argc,                                            \
-	    MUNIT_UNUSED char *const argv[MUNIT_ARRAY_PARAM(argc)])
-
-#define CURRENT_ARG (argv[*arg])
-#define NEXT_ARG (argc > *arg + 1 ? argv[*arg + 1] : NULL)
-#define SHIFT (*arg)++
-
 /* Declare and register a new test suite #S belonging to the file's test module.
  *
  * A test suite is a pair of static variables: