New test: reveals race (found by Volker) when open occurs during transaction commit.

ccan/tdb/test/external-transaction.c

@@ -7,7 +7,9 @@
 #include <stdlib.h>
 #include <limits.h>
 #include <string.h>
+#include <errno.h>
 #include <ccan/tdb/tdb.h>
+#include <ccan/tap/tap.h>
 
 static volatile sig_atomic_t alarmed;
 static void do_alarm(int signum)
@@ -15,15 +17,22 @@ static void do_alarm(int signum)
 	alarmed++;
 }
 
-static int do_transaction(const char *name)
+static int do_operation(enum operation op, const char *name)
 {
 	TDB_DATA k = { .dptr = (void *)"a", .dsize = 1 };
 	TDB_DATA d = { .dptr = (void *)"b", .dsize = 1 };
-	struct tdb_context *tdb = tdb_open(name, 0, 0, O_RDWR, 0);
+	struct tdb_context *tdb;
 
+	tdb = tdb_open(name, 0, op == OPEN_WITH_CLEAR_IF_FIRST ?
+		       TDB_CLEAR_IF_FIRST : TDB_DEFAULT, O_RDWR, 0);
 	if (!tdb)
 		return -1;
 
+	if (op == OPEN || op == OPEN_WITH_CLEAR_IF_FIRST) {
+		tdb_close(tdb);
+		return 0;
+	}
+
 	alarmed = 0;
 	tdb_setalarm_sigptr(tdb, &alarmed);
 
@@ -61,7 +70,7 @@ struct agent *prepare_external_agent(void)
 	int pid;
 	int command[2], response[2];
 	struct sigaction act = { .sa_handler = do_alarm };
-	char name[PATH_MAX];
+	char name[1+PATH_MAX];
 
 	if (pipe(command) != 0 || pipe(response) != 0)
 		return NULL;
@@ -85,7 +94,9 @@ struct agent *prepare_external_agent(void)
 	sigaction(SIGALRM, &act, NULL);
 
 	while (read(command[0], name, sizeof(name)) != 0) {
-		int result = do_transaction(name);
+		int result;
+
+		result = do_operation(name[0], name+1);
 		if (write(response[1], &result, sizeof(result))
 		    != sizeof(result))
 			err(1, "Writing response");
@@ -93,13 +104,17 @@ struct agent *prepare_external_agent(void)
 	exit(0);
 }
 
-/* Ask the external agent to try to do a transaction. */
-bool external_agent_transaction(struct agent *agent, const char *tdbname)
+/* Ask the external agent to try to do an operation. */
+bool external_agent_operation(struct agent *agent,
+			      enum operation op, const char *tdbname)
 {
 	int res;
+	char string[1 + strlen(tdbname) + 1];
 
-	if (write(agent->cmdfd, tdbname, strlen(tdbname)+1)
-	    != strlen(tdbname)+1)
+	string[0] = op;
+	strcpy(string+1, tdbname);
+
+	if (write(agent->cmdfd, string, sizeof(string)) != sizeof(string))
 		err(1, "Writing to agent");
 
 	if (read(agent->responsefd, &res, sizeof(res)) != sizeof(res))
@@ -110,3 +125,41 @@ bool external_agent_transaction(struct agent *agent, const char *tdbname)
 
 	return res;
 }
+
+void external_agent_operation_start(struct agent *agent,
+				    enum operation op, const char *tdbname)
+{
+	char string[1 + strlen(tdbname) + 1];
+
+	string[0] = op;
+	strcpy(string+1, tdbname);
+
+	if (write(agent->cmdfd, string, sizeof(string)) != sizeof(string))
+		err(1, "Writing to agent");
+}
+
+bool external_agent_operation_check(struct agent *agent, bool block, int *res)
+{
+	int flags = fcntl(agent->responsefd, F_GETFL);
+
+	if (block)
+		fcntl(agent->responsefd, F_SETFL, flags & ~O_NONBLOCK);
+	else
+		fcntl(agent->responsefd, F_SETFL, flags | O_NONBLOCK);
+
+	switch (read(agent->responsefd, res, sizeof(*res))) {
+	case sizeof(*res):
+		break;
+	case 0:
+		errx(1, "Agent died?");
+	default:
+		if (!block && (errno == EAGAIN || errno == EWOULDBLOCK))
+			return false;
+		err(1, "%slocking reading from agent", block ? "B" : "Non-b");
+	}
+
+	if (*res > 1)
+		errx(1, "Agent returned %u\n", *res);
+
+	return true;
+}

ccan/tdb/test/external-transaction.h

@@ -2,10 +2,24 @@
 #define TDB_TEST_EXTERNAL_TRANSACTION_H
 #include <stdbool.h>
 
+enum operation {
+	OPEN,
+	OPEN_WITH_CLEAR_IF_FIRST,
+	TRANSACTION,
+};
+
 /* Do this before doing any tdb stuff.  Return handle, or -1. */
 struct agent *prepare_external_agent(void);
 
-/* Ask the external agent to try to do a transaction. */
-bool external_agent_transaction(struct agent *handle, const char *tdbname);
+/* Ask the external agent to try to do an operation. */
+bool external_agent_operation(struct agent *handle,
+			      enum operation op,
+			      const char *tdbname);
+
+/* Ask... */
+void external_agent_operation_start(struct agent *agent,
+				    enum operation op, const char *tdbname);
 
+/* See if they've done it. */
+bool external_agent_operation_check(struct agent *agent, bool block, int *res);
 #endif /* TDB_TEST_EXTERNAL_TRANSACTION_H */

ccan/tdb/test/run-nested-traverse.c

@@ -41,11 +41,11 @@ static int traverse1(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
 {
 	ok1(correct_key(key));
 	ok1(correct_data(data));
-	ok1(!external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 	tdb_traverse(tdb, traverse2, NULL);
 
 	/* That should *not* release the transaction lock! */
-	ok1(!external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 	return 0;
 }
 
@@ -63,7 +63,7 @@ int main(int argc, char *argv[])
 		       O_CREAT|O_TRUNC|O_RDWR, 0600);
 	ok1(tdb);
 
-	ok1(external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 
 	key.dsize = strlen("hi");
 	key.dptr = (void *)"hi";

ccan/tdb/test/run-open-during-transaction.c

+#define _XOPEN_SOURCE 500
+#include <unistd.h>
+static ssize_t pwrite_check(int fd, const void *buf, size_t count, off_t offset);
+static ssize_t write_check(int fd, const void *buf, size_t count);
+static int fcntl_check(int fd, int cmd, ... /* arg */ );
+static int ftruncate_check(int fd, off_t length);
+
+#define pwrite pwrite_check
+#define write write_check
+#define fcntl fcntl_check
+#define ftruncate ftruncate_check
+
+#include <ccan/tdb/tdb.h>
+#include <ccan/tdb/io.c>
+#include <ccan/tdb/tdb.c>
+#include <ccan/tdb/lock.c>
+#include <ccan/tdb/freelist.c>
+#include <ccan/tdb/traverse.c>
+#include <ccan/tdb/transaction.c>
+#include <ccan/tdb/error.c>
+#include <ccan/tdb/open.c>
+#include <ccan/tap/tap.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdarg.h>
+#include <err.h>
+#include "external-transaction.h"
+
+static struct agent *agent;
+static bool agent_pending;
+static bool in_transaction;
+static int errors = 0;
+static bool snapshot_uptodate;
+static char *snapshot;
+static off_t snapshot_len;
+static bool clear_if_first;
+#define TEST_DBNAME "/tmp/test7.tdb"
+
+#undef write
+#undef pwrite
+#undef fcntl
+#undef ftruncate
+
+static void save_file_contents(int fd)
+{
+	struct stat st;
+	int res;
+
+	/* Save copy of file. */
+	stat(TEST_DBNAME, &st);
+	if (snapshot_len != st.st_size) {
+		snapshot = realloc(snapshot, st.st_size * 2);
+		snapshot_len = st.st_size;
+	}
+	res = pread(fd, snapshot, snapshot_len, 0);
+	if (res != snapshot_len)
+		err(1, "Reading %zu bytes = %u", (size_t)snapshot_len, res);
+	snapshot_uptodate = true;
+}
+
+static void check_for_agent(int fd, bool block)
+{
+	struct stat st;
+	int res;
+
+	if (!external_agent_operation_check(agent, block, &res))
+		return;
+
+	if (res != 0)
+		err(1, "Agent failed open");
+	agent_pending = false;
+
+	if (!snapshot_uptodate)
+		return;
+
+	stat(TEST_DBNAME, &st);
+	if (st.st_size != snapshot_len) {
+		diag("Other open changed size from %zu -> %zu",
+		     (size_t)snapshot_len, (size_t)st.st_size);
+		errors++;
+		return;
+	}
+
+	if (pread(fd, snapshot+snapshot_len, snapshot_len, 0) != snapshot_len)
+		err(1, "Reading %zu bytes", (size_t)snapshot_len);
+	if (memcmp(snapshot, snapshot+snapshot_len, snapshot_len) != 0) {
+		diag("File changed");
+		errors++;
+		return;
+	}
+}
+
+static void check_file_contents(int fd)
+{
+	if (agent_pending)
+		check_for_agent(fd, false);
+
+	if (!agent_pending) {
+		save_file_contents(fd);
+
+		/* Ask agent to open file. */
+		external_agent_operation_start(agent,
+					       clear_if_first ?
+					       OPEN_WITH_CLEAR_IF_FIRST :
+					       OPEN,
+					       TEST_DBNAME);
+		agent_pending = true;
+		/* Hack: give it a chance to run. */
+		sleep(0);
+	}
+
+	check_for_agent(fd, false);
+}
+
+static ssize_t pwrite_check(int fd,
+			    const void *buf, size_t count, off_t offset)
+{
+	ssize_t ret;
+
+	if (in_transaction)
+		check_file_contents(fd);
+
+	snapshot_uptodate = false;
+	ret = pwrite(fd, buf, count, offset);
+	if (ret != count)
+		return ret;
+
+	if (in_transaction)
+		check_file_contents(fd);
+	return ret;
+}
+
+static ssize_t write_check(int fd, const void *buf, size_t count)
+{
+	ssize_t ret;
+
+	if (in_transaction)
+		check_file_contents(fd);
+
+	snapshot_uptodate = false;
+
+	ret = write(fd, buf, count);
+	if (ret != count)
+		return ret;
+
+	if (in_transaction)
+		check_file_contents(fd);
+	return ret;
+}
+
+/* This seems to be a macro for glibc... */
+extern int fcntl(int fd, int cmd, ... /* arg */ );
+
+static int fcntl_check(int fd, int cmd, ... /* arg */ )
+{
+	va_list ap;
+	int ret, arg3;
+	struct flock *fl;
+
+	if (cmd != F_SETLK && cmd != F_SETLKW) {
+		/* This may be totally bogus, but we don't know in general. */
+		va_start(ap, cmd);
+		arg3 = va_arg(ap, int);
+		va_end(ap);
+
+		return fcntl(fd, cmd, arg3);
+	}
+
+	va_start(ap, cmd);
+	fl = va_arg(ap, struct flock *);
+	va_end(ap);
+
+	ret = fcntl(fd, cmd, fl);
+
+	if (in_transaction && fl->l_type == F_UNLCK)
+		check_file_contents(fd);
+	return ret;
+}
+
+static int ftruncate_check(int fd, off_t length)
+{
+	int ret;
+
+	if (in_transaction)
+		check_file_contents(fd);
+
+	snapshot_uptodate = false;
+
+	ret = ftruncate(fd, length);
+
+	if (in_transaction)
+		check_file_contents(fd);
+	return ret;
+}
+
+int main(int argc, char *argv[])
+{
+	const int flags[] = { TDB_DEFAULT,
+			      TDB_CLEAR_IF_FIRST,
+			      TDB_NOMMAP, 
+			      TDB_CLEAR_IF_FIRST | TDB_NOMMAP };
+	int i;
+	struct tdb_context *tdb;
+	TDB_DATA key, data;
+
+	plan_tests(20);
+	agent = prepare_external_agent();
+	if (!agent)
+		err(1, "preparing agent");
+
+	/* Nice ourselves down: we can't tell the difference between agent
+	 * blocking on lock, and agent not scheduled. */
+	nice(15);
+
+	for (i = 0; i < sizeof(flags)/sizeof(flags[0]); i++) {
+		unlink(TEST_DBNAME);
+		tdb = tdb_open(TEST_DBNAME, 1024, flags[i],
+			       O_CREAT|O_TRUNC|O_RDWR, 0600);
+		ok1(tdb);
+		clear_if_first = (flags[i] & TDB_CLEAR_IF_FIRST);
+
+		ok1(tdb_transaction_start(tdb) == 0);
+		in_transaction = true;
+		key.dsize = strlen("hi");
+		key.dptr = (void *)"hi";
+		data.dptr = (void *)"world";
+		data.dsize = strlen("world");
+
+		ok1(tdb_store(tdb, key, data, TDB_INSERT) == 0);
+		ok1(tdb_transaction_commit(tdb) == 0);
+		if (agent_pending)
+			check_for_agent(tdb->fd, true);
+		ok(errors == 0, "We had %u errors", errors);
+
+		tdb_close(tdb);
+	}
+
+	return exit_status();
+}

ccan/tdb/test/run-traverse-in-transaction.c

@@ -57,21 +57,21 @@ int main(int argc, char *argv[])
 
 	ok1(tdb_store(tdb, key, data, TDB_INSERT) == 0);
 
-	ok1(external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 
 	ok1(tdb_transaction_start(tdb) == 0);
-	ok1(!external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 	tdb_traverse(tdb, traverse, NULL);
 
 	/* That should *not* release the transaction lock! */
-	ok1(!external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 	tdb_traverse_read(tdb, traverse, NULL);
 
 	/* That should *not* release the transaction lock! */
-	ok1(!external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 	ok1(tdb_transaction_commit(tdb) == 0);
 	/* Now we should be fine. */
-	ok1(external_agent_transaction(agent, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
 
 	tdb_close(tdb);