tdb: rewrite external agent for testing.

For locking and transaction tests, we need an external process to probe the
tdb.  This code was a mess, and unreliable (occasional failures).  Rewrite
it so that instead of blocking and using SIGALRM, we mug fcntl and force
non-blocking locks.  We use a special return to day "I couldn't because
the lock wasn't available".

I also made the operations clearer and more orthogonal rather than "what
we needed for each test" which was really hard to understand.

It turns out those occasional failures weren't spurious.  Next patch!

ccan/tdb/test/external-agent.c

@@ -0,0 +1,209 @@
+#include "external-agent.h"
+#include "lock-tracking.h"
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <err.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <limits.h>
+#include <string.h>
+#include <errno.h>
+#include <ccan/tdb/tdb.h>
+#include <ccan/tdb/tdb_private.h>
+#include <ccan/tap/tap.h>
+#include <stdio.h>
+#include <stdarg.h>
+
+static struct tdb_context *tdb;
+
+static void taplog(struct tdb_context *tdb,
+		   enum tdb_debug_level level,
+		   const char *fmt, ...)
+{
+	va_list ap;
+	char line[200];
+
+	va_start(ap, fmt);
+	vsprintf(line, fmt, ap);
+	va_end(ap);
+
+	diag("external: %s", line);
+}
+
+static enum agent_return do_operation(enum operation op, const char *name)
+{
+	struct tdb_logging_context logctx = { taplog, NULL };
+	TDB_DATA k;
+	enum agent_return ret;
+	TDB_DATA data;
+
+	if (op != OPEN && op != OPEN_WITH_CLEAR_IF_FIRST && !tdb) {
+		diag("external: No tdb open!");
+		return OTHER_FAILURE;
+	}
+
+	k.dptr = (void *)name;
+	k.dsize = strlen(name);
+
+	locking_would_block = 0;
+	switch (op) {
+	case OPEN:
+		if (tdb) {
+			diag("Already have tdb %s open", tdb_name(tdb));
+			return OTHER_FAILURE;
+		}
+		tdb = tdb_open_ex(name, 0, TDB_DEFAULT, O_RDWR, 0,
+				  &logctx, NULL);
+		if (!tdb) {
+			if (!locking_would_block)
+				diag("Opening tdb gave %s", strerror(errno));
+			ret = OTHER_FAILURE;
+		} else
+			ret = SUCCESS;
+		break;
+	case OPEN_WITH_CLEAR_IF_FIRST:
+		if (tdb)
+			return OTHER_FAILURE;
+		tdb = tdb_open_ex(name, 0, TDB_CLEAR_IF_FIRST, O_RDWR, 0,
+				  &logctx, NULL);
+		ret = tdb ? SUCCESS : OTHER_FAILURE;
+		break;
+	case TRANSACTION_START:
+		ret = tdb_transaction_start(tdb) == 0 ? SUCCESS : OTHER_FAILURE;
+		break;
+	case FETCH:
+		data = tdb_fetch(tdb, k);
+		if (data.dptr == NULL) {
+			if (tdb_error(tdb) == TDB_ERR_NOEXIST)
+				ret = FAILED;
+			else
+				ret = OTHER_FAILURE;
+		} else if (data.dsize != k.dsize
+			   || memcmp(data.dptr, k.dptr, k.dsize) != 0) {
+			ret = OTHER_FAILURE;
+		} else {
+			ret = SUCCESS;
+		}
+		free(data.dptr);
+		break;
+	case STORE:
+		ret = tdb_store(tdb, k, k, 0) == 0 ? SUCCESS : OTHER_FAILURE;
+		break;
+	case TRANSACTION_COMMIT:
+		ret = tdb_transaction_commit(tdb)==0 ? SUCCESS : OTHER_FAILURE;
+		break;
+	case CHECK:
+		ret = tdb_check(tdb, NULL, NULL) == 0 ? SUCCESS : OTHER_FAILURE;
+		break;
+	case NEEDS_RECOVERY:
+		ret = tdb_needs_recovery(tdb) ? SUCCESS : FAILED;
+		break;
+	case CLOSE:
+		ret = tdb_close(tdb) == 0 ? SUCCESS : OTHER_FAILURE;
+		tdb = NULL;
+		break;
+	default:
+		ret = OTHER_FAILURE;
+	}
+
+	if (locking_would_block)
+		ret = WOULD_HAVE_BLOCKED;
+
+	return ret;
+}
+
+struct agent {
+	int cmdfd, responsefd;
+};
+
+/* Do this before doing any tdb stuff.  Return handle, or NULL. */
+struct agent *prepare_external_agent(void)
+{
+	int pid, ret;
+	int command[2], response[2];
+	char name[1+PATH_MAX];
+
+	if (pipe(command) != 0 || pipe(response) != 0)
+		return NULL;
+
+	pid = fork();
+	if (pid < 0)
+		return NULL;
+
+	if (pid != 0) {
+		struct agent *agent = malloc(sizeof(*agent));
+
+		close(command[0]);
+		close(response[1]);
+		agent->cmdfd = command[1];
+		agent->responsefd = response[0];
+		return agent;
+	}
+
+	close(command[1]);
+	close(response[0]);
+
+	/* We want to fail, not block. */
+	nonblocking_locks = true;
+	while ((ret = read(command[0], name, sizeof(name))) > 0) {
+		enum agent_return result;
+
+		result = do_operation(name[0], name+1);
+		if (write(response[1], &result, sizeof(result))
+		    != sizeof(result))
+			err(1, "Writing response");
+	}
+	exit(0);
+}
+
+/* Ask the external agent to try to do an operation. */
+enum agent_return external_agent_operation(struct agent *agent,
+					   enum operation op,
+					   const char *name)
+{
+	enum agent_return res;
+	unsigned int len;
+	char *string;
+
+	if (!name)
+		name = "";
+	len = 1 + strlen(name) + 1;
+	string = malloc(len);
+
+	string[0] = op;
+	strcpy(string+1, name);
+
+	if (write(agent->cmdfd, string, len) != len
+	    || read(agent->responsefd, &res, sizeof(res)) != sizeof(res))
+		res = AGENT_DIED;
+
+	free(string);
+	return res;
+}
+
+const char *agent_return_name(enum agent_return ret)
+{
+	return ret == SUCCESS ? "SUCCESS"
+		: ret == WOULD_HAVE_BLOCKED ? "WOULD_HAVE_BLOCKED"
+		: ret == AGENT_DIED ? "AGENT_DIED"
+		: ret == FAILED ? "FAILED"
+		: ret == OTHER_FAILURE ? "OTHER_FAILURE"
+		: "**INVALID**";
+}
+
+const char *operation_name(enum operation op)
+{
+	switch (op) {
+	case OPEN: return "OPEN";
+	case OPEN_WITH_CLEAR_IF_FIRST: return "OPEN_WITH_CLEAR_IF_FIRST";
+	case TRANSACTION_START: return "TRANSACTION_START";
+	case FETCH: return "FETCH";
+	case STORE: return "STORE";
+	case TRANSACTION_COMMIT: return "TRANSACTION_COMMIT";
+	case CHECK: return "CHECK";
+	case NEEDS_RECOVERY: return "NEEDS_RECOVERY";
+	case CLOSE: return "CLOSE";
+	}
+	return "**INVALID**";
+}

ccan/tdb/test/external-agent.h

@@ -0,0 +1,41 @@
+#ifndef TDB_TEST_EXTERNAL_AGENT_H
+#define TDB_TEST_EXTERNAL_AGENT_H
+
+/* For locking tests, we need a different process to try things at
+ * various times. */
+enum operation {
+	OPEN,
+	OPEN_WITH_CLEAR_IF_FIRST,
+	TRANSACTION_START,
+	FETCH,
+	STORE,
+	TRANSACTION_COMMIT,
+	CHECK,
+	NEEDS_RECOVERY,
+	CLOSE,
+};
+
+/* Do this before doing any tdb stuff.  Return handle, or -1. */
+struct agent *prepare_external_agent(void);
+
+enum agent_return {
+	SUCCESS,
+	WOULD_HAVE_BLOCKED,
+	AGENT_DIED,
+	FAILED, /* For fetch, or NEEDS_RECOVERY */
+	OTHER_FAILURE,
+};
+
+/* Ask the external agent to try to do an operation. 
+ * name == tdb name for OPEN/OPEN_WITH_CLEAR_IF_FIRST,
+ * record name for FETCH/STORE (store stores name as data too)
+ */
+enum agent_return external_agent_operation(struct agent *handle,
+					   enum operation op,
+					   const char *name);
+
+/* Mapping enum -> string. */
+const char *agent_return_name(enum agent_return ret);
+const char *operation_name(enum operation op);
+
+#endif /* TDB_TEST_EXTERNAL_AGENT_H */

ccan/tdb/test/external-transaction.c

@@ -1,220 +0,0 @@
-#include "external-transaction.h"
-#include <sys/types.h>
-#include <sys/wait.h>
-#include <unistd.h>
-#include <err.h>
-#include <fcntl.h>
-#include <stdlib.h>
-#include <limits.h>
-#include <string.h>
-#include <errno.h>
-#include <ccan/tdb/tdb.h>
-#include <ccan/tdb/tdb_private.h>
-#include <ccan/tap/tap.h>
-#include <stdio.h>
-#include <stdarg.h>
-
-static struct tdb_context *tdb;
-
-static volatile sig_atomic_t alarmed;
-static void do_alarm(int signum)
-{
-	alarmed++;
-}
-
-static void taplog(struct tdb_context *tdb,
-		   enum tdb_debug_level level,
-		   const char *fmt, ...)
-{
-	va_list ap;
-	char line[200];
-
-	va_start(ap, fmt);
-	vsprintf(line, fmt, ap);
-	va_end(ap);
-
-	diag("external: %s", line);
-}
-
-static int do_operation(enum operation op, const char *name)
-{
-	struct tdb_logging_context logctx = { taplog, NULL };
-
-	TDB_DATA k = { .dptr = (void *)"a", .dsize = 1 };
-	TDB_DATA d = { .dptr = (void *)"b", .dsize = 1 };
-
-	if (op <= KEEP_OPENED) {
-		tdb = tdb_open_ex(name, 0, op == OPEN_WITH_CLEAR_IF_FIRST ?
-				  TDB_CLEAR_IF_FIRST : TDB_DEFAULT, O_RDWR, 0,
-				  &logctx, NULL);
-		if (!tdb)
-			return -1;
-	}
-
-	if (op == KEEP_OPENED) {
-		return 0;
-	} else if (op == OPEN || op == OPEN_WITH_CLEAR_IF_FIRST || op == CLOSE) {
-		tdb_close(tdb);
-		tdb = NULL;
-		return 1;
-	} else if (op == STORE_KEEP_OPENED) {
-		if (tdb_store(tdb, k, d, 0) != 0)
-			return -2;
-		return 1;
-	} else if (op == FETCH_KEEP_OPENED) {
-		TDB_DATA ret;
-		ret = tdb_fetch(tdb, k);
-		if (ret.dptr == NULL) {
-			if (tdb_error(tdb) == TDB_ERR_NOEXIST)
-				return 1;
-			return -3;
-		}
-		if (ret.dsize != 1 || *(char *)ret.dptr != 'b')
-			return -4;
-		free(ret.dptr);
-		return 1;
-	} else if (op == CHECK_KEEP_OPENED) {
-		return tdb_check(tdb, NULL, 0) == 0;
-	} else if (op == NEEDS_RECOVERY_KEEP_OPENED) {
-		return tdb_needs_recovery(tdb);
-	}
-
-	alarmed = 0;
-	tdb_setalarm_sigptr(tdb, &alarmed);
-
-	alarm(1);
-	if (tdb_transaction_start(tdb) != 0)
-		goto maybe_alarmed;
-
-	alarm(0);
-	if (tdb_store(tdb, k, d, 0) != 0) {
-		tdb_transaction_cancel(tdb);
-		tdb_close(tdb);
-		tdb = NULL;
-		return -2;
-	}
-
-	if (tdb_transaction_commit(tdb) == 0) {
-		tdb_delete(tdb, k);
-		if (op != TRANSACTION_KEEP_OPENED) {
-			tdb_close(tdb);
-			tdb = NULL;
-		}
-		return 1;
-	}
-
-	tdb_delete(tdb, k);
-maybe_alarmed:
-	if (op != TRANSACTION_KEEP_OPENED) {
-		tdb_close(tdb);
-		tdb = NULL;
-	}
-	if (alarmed)
-		return 0;
-	return -3;
-}
-
-struct agent {
-	int cmdfd, responsefd;
-};
-
-/* Do this before doing any tdb stuff.  Return handle, or NULL. */
-struct agent *prepare_external_agent(void)
-{
-	int pid, ret;
-	int command[2], response[2];
-	struct sigaction act = { .sa_handler = do_alarm };
-	char name[1+PATH_MAX];
-
-	if (pipe(command) != 0 || pipe(response) != 0)
-		return NULL;
-
-	pid = fork();
-	if (pid < 0)
-		return NULL;
-
-	if (pid != 0) {
-		struct agent *agent = malloc(sizeof(*agent));
-
-		close(command[0]);
-		close(response[1]);
-		agent->cmdfd = command[1];
-		agent->responsefd = response[0];
-		return agent;
-	}
-
-	close(command[1]);
-	close(response[0]);
-	sigaction(SIGALRM, &act, NULL);
-
-	while ((ret = read(command[0], name, sizeof(name))) > 0) {
-		int result;
-
-		result = do_operation(name[0], name+1);
-		if (write(response[1], &result, sizeof(result))
-		    != sizeof(result))
-			err(1, "Writing response");
-	}
-	diag("external: read %i: %s", ret, strerror(errno));
-	exit(0);
-}
-
-/* Ask the external agent to try to do an operation. */
-int external_agent_operation(struct agent *agent,
-			      enum operation op, const char *tdbname)
-{
-	int res;
-	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");
-
-	if (read(agent->responsefd, &res, sizeof(res)) != sizeof(res))
-		err(1, "Reading from agent");
-
-	if (res > 1)
-		errx(1, "Agent returned %u\n", res);
-
-	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

@@ -1,32 +0,0 @@
-#ifndef TDB_TEST_EXTERNAL_TRANSACTION_H
-#define TDB_TEST_EXTERNAL_TRANSACTION_H
-#include <stdbool.h>
-
-enum operation {
-	OPEN,
-	OPEN_WITH_CLEAR_IF_FIRST,
-	TRANSACTION,
-	KEEP_OPENED,
-	TRANSACTION_KEEP_OPENED,
-	FETCH_KEEP_OPENED,
-	STORE_KEEP_OPENED,
-	CHECK_KEEP_OPENED,
-	NEEDS_RECOVERY_KEEP_OPENED,
-	CLOSE,
-};
-
-/* 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 an operation. */
-int 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/lock-tracking.c

@@ -15,6 +15,8 @@ struct lock {
 static struct lock *locks;
 int locking_errors = 0;
 bool suppress_lockcheck = false;
+bool nonblocking_locks;
+int locking_would_block = 0;
 void (*unlock_callback)(int fd);
 
 int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ )
@@ -22,6 +24,7 @@ int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ )
 	va_list ap;
 	int ret, arg3;
 	struct flock *fl;
+	bool may_block = false;
 
 	if (cmd != F_SETLK && cmd != F_SETLKW) {
 		/* This may be totally bogus, but we don't know in general. */
@@ -36,6 +39,17 @@ int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ )
 	fl = va_arg(ap, struct flock *);
 	va_end(ap);
 
+	if (cmd == F_SETLKW && nonblocking_locks) {
+		cmd = F_SETLK;
+		may_block = true;
+	}
+	ret = fcntl(fd, cmd, fl);
+
+	/* Detect when we failed, but might have been OK if we waited. */
+	if (may_block && ret == -1 && (errno == EAGAIN || errno == EACCES)) {
+		locking_would_block++;
+	}
+
 	if (fl->l_type == F_UNLCK) {
 		struct lock **l;
 		struct lock *old = NULL;
@@ -43,15 +57,17 @@ int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ )
 		for (l = &locks; *l; l = &(*l)->next) {
 			if ((*l)->off == fl->l_start
 			    && (*l)->len == fl->l_len) {
-				old = *l;
-				*l = (*l)->next;
-				free(old);
+				if (ret == 0) {
+					old = *l;
+					*l = (*l)->next;
+					free(old);
+				}
 				break;
 			}
 		}
 		if (!old && !suppress_lockcheck) {
-			diag("Unknown unlock %u@%u",
-			     (int)fl->l_len, (int)fl->l_start);
+			diag("Unknown unlock %u@%u - %i",
+			     (int)fl->l_len, (int)fl->l_start, ret);
 			locking_errors++;
 		}
 	} else {
@@ -73,8 +89,12 @@ int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ )
 
 			/* tdb_allrecord_lock does this, handle adjacent: */
 			if (fl->l_start == i_end && fl->l_type == i->type) {
-				i->len = fl->l_len ? i->len + fl->l_len : 0;
-				goto ok;
+				if (ret == 0) {
+					i->len = fl->l_len 
+						? i->len + fl->l_len
+						: 0;
+				}
+				goto done;
 			}
 		}
 		if (i) {
@@ -84,8 +104,9 @@ int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ )
 			    && fl->l_start == FREELIST_TOP
 			    && i->len == 0
 			    && fl->l_len == 0) {
-				i->type = F_WRLCK;
-				goto ok;
+				if (ret == 0)
+					i->type = F_WRLCK;
+				goto done;
 			}
 			if (!suppress_lockcheck) {
 				diag("%s lock %u@%u overlaps %u@%u",
@@ -95,15 +116,17 @@ int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ )
 				locking_errors++;
 			}
 		}
-		new = malloc(sizeof *new);
-		new->off = fl->l_start;
-		new->len = fl->l_len;
-		new->type = fl->l_type;
-		new->next = locks;
-		locks = new;
+
+		if (ret == 0) {
+			new = malloc(sizeof *new);
+			new->off = fl->l_start;
+			new->len = fl->l_len;
+			new->type = fl->l_type;
+			new->next = locks;
+			locks = new;
+		}
 	}
-ok:
-	ret = fcntl(fd, cmd, fl);
+done:
 	if (ret == 0 && fl->l_type == F_UNLCK && unlock_callback)
 		unlock_callback(fd);
 	return ret;

ccan/tdb/test/lock-tracking.h

@@ -16,4 +16,10 @@ extern int locking_errors;
 
 /* Suppress lock checking. */
 extern bool suppress_lockcheck;
+
+/* Make all locks non-blocking. */
+extern bool nonblocking_locks;
+
+/* Number of times we failed a lock because we made it non-blocking. */
+extern int locking_would_block;
 #endif /* LOCK_TRACKING_H */

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

@@ -26,7 +26,7 @@ static int ftruncate_check(int fd, off_t length);
 #include <stdarg.h>
 #include <err.h>
 #include <setjmp.h>
-#include "external-transaction.h"
+#include "external-agent.h"
 
 #undef write
 #undef pwrite
@@ -38,6 +38,7 @@ static bool suppress_logging;
 static int target, current;
 static jmp_buf jmpbuf;
 #define TEST_DBNAME "run-die-during-transaction.tdb"
+#define KEY_STRING "helloworld"
 
 static void taplog(struct tdb_context *tdb,
 		   enum tdb_debug_level level,
@@ -107,8 +108,9 @@ static int ftruncate_check(int fd, off_t length)
 static bool test_death(enum operation op, struct agent *agent)
 {
 	struct tdb_context *tdb = NULL;
-	TDB_DATA key, data;
+	TDB_DATA key;
 	struct tdb_logging_context logctx = { taplog, NULL };
+	enum agent_return ret;
 	int needed_recovery = 0;
 
 	current = target = 0;
@@ -119,30 +121,44 @@ reset:
 		forget_locking();
 		in_transaction = false;
 
-		if (external_agent_operation(agent, NEEDS_RECOVERY_KEEP_OPENED,
-					     ""))
+		ret = external_agent_operation(agent, NEEDS_RECOVERY, "");
+		if (ret == SUCCESS)
 			needed_recovery++;
+		else if (ret != FAILED) {
+			diag("Step %u agent NEEDS_RECOVERY = %s", current,
+			     agent_return_name(ret));
+			return false;
+		}
+
+		ret = external_agent_operation(agent, op, KEY_STRING);
+		if (ret != SUCCESS) {
+			diag("Step %u op %s failed = %s", current,
+			     operation_name(op),
+			     agent_return_name(ret));
+			return false;
+		}
 
-		if (external_agent_operation(agent, op, "") != 1) {
-			diag("Step %u op failed", current);
+		ret = external_agent_operation(agent, NEEDS_RECOVERY, "");
+		if (ret != FAILED) {
+			diag("Still needs recovery after step %u = %s",
+			     current, agent_return_name(ret));
 			return false;
 		}
 
-		if (external_agent_operation(agent, NEEDS_RECOVERY_KEEP_OPENED,
-					     "")) {
-			diag("Still needs recovery after step %u", current);
+		ret = external_agent_operation(agent, CHECK, "");
+		if (ret != SUCCESS) {
+			diag("Step %u check failed = %s", current,
+			     agent_return_name(ret));
 			return false;
 		}
 
-		if (external_agent_operation(agent, CHECK_KEEP_OPENED, "")
-		    != 1) {
-			diag("Step %u check failed", current);
-#if 0
+		ret = external_agent_operation(agent, CLOSE, "");
+		if (ret != SUCCESS) {
+			diag("Step %u close failed = %s", current,
+			     agent_return_name(ret));
 			return false;
-#endif
 		}
 
-		external_agent_operation(agent, CLOSE, "");
 		/* Suppress logging as this tries to use closed fd. */
 		suppress_logging = true;
 		suppress_lockcheck = true;
@@ -158,20 +174,30 @@ reset:
 	tdb = tdb_open_ex(TEST_DBNAME, 1024, TDB_NOMMAP,
 			  O_CREAT|O_TRUNC|O_RDWR, 0600, &logctx, NULL);
 
-	if (external_agent_operation(agent, KEEP_OPENED, TEST_DBNAME) != 0)
-		errx(1, "Agent failed to open?");
-
-	if (tdb_transaction_start(tdb) != 0)
+	/* Put key for agent to fetch. */
+	key.dsize = strlen(KEY_STRING);
+	key.dptr = (void *)KEY_STRING;
+	if (tdb_store(tdb, key, key, TDB_INSERT) != 0)
 		return false;
 
+	/* This is the key we insert in transaction. */
+	key.dsize--;
+
+	ret = external_agent_operation(agent, OPEN, TEST_DBNAME);
+	if (ret != SUCCESS)
+		errx(1, "Agent failed to open: %s", agent_return_name(ret));
+
+	ret = external_agent_operation(agent, FETCH, KEY_STRING);
+	if (ret != SUCCESS)
+		errx(1, "Agent failed find key: %s", agent_return_name(ret));
+
 	in_transaction = true;
-	key.dsize = strlen("hi");
-	key.dptr = (void *)"hi";
-	data.dptr = (void *)"world";
-	data.dsize = strlen("world");
+	if (tdb_transaction_start(tdb) != 0)
+		return false;
 
-	if (tdb_store(tdb, key, data, TDB_INSERT) != 0)
+	if (tdb_store(tdb, key, key, TDB_INSERT) != 0)
 		return false;
+
 	if (tdb_transaction_commit(tdb) != 0)
 		return false;
 
@@ -180,7 +206,12 @@ reset:
 	/* We made it! */
 	diag("Completed %u runs", current);
 	tdb_close(tdb);
-	external_agent_operation(agent, CLOSE, "");
+	ret = external_agent_operation(agent, CLOSE, "");
+	if (ret != SUCCESS) {
+		diag("Step %u close failed = %s", current,
+		     agent_return_name(ret));
+		return false;
+	}
 
 	ok1(needed_recovery);
 	ok1(locking_errors == 0);
@@ -191,9 +222,7 @@ reset:
 
 int main(int argc, char *argv[])
 {
-	enum operation ops[] = { FETCH_KEEP_OPENED,
-				 STORE_KEEP_OPENED,
-				 TRANSACTION_KEEP_OPENED };
+	enum operation ops[] = { FETCH, STORE, TRANSACTION_START };
 	struct agent *agent;
 	int i;
 
@@ -204,17 +233,8 @@ int main(int argc, char *argv[])
 	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(ops)/sizeof(ops[0]); i++) {
-		diag("Testing %s after death",
-		     ops[i] == TRANSACTION_KEEP_OPENED ? "transaction"
-		     : ops[i] == FETCH_KEEP_OPENED ? "fetch"
-		     : ops[i] == STORE_KEEP_OPENED ? "store"
-		     : NULL);
-
+		diag("Testing %s after death", operation_name(ops[i]));
 		ok1(test_death(ops[i], agent));
 	}
 

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

@@ -1,4 +1,6 @@
 #define _XOPEN_SOURCE 500
+#include "lock-tracking.h"
+#define fcntl fcntl_with_lockcheck
 #include <ccan/tdb/tdb.h>
 #include <ccan/tdb/io.c>
 #include <ccan/tdb/tdb.c>
@@ -10,10 +12,11 @@
 #include <ccan/tdb/open.c>
 #include <ccan/tdb/check.c>
 #include <ccan/tap/tap.h>
+#undef fcntl
 #include <stdlib.h>
 #include <stdbool.h>
 #include <err.h>
-#include "external-transaction.h"
+#include "external-agent.h"
 
 static struct agent *agent;
 
@@ -42,11 +45,13 @@ static int traverse1(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
 {
 	ok1(correct_key(key));
 	ok1(correct_data(data));
-	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+	    == WOULD_HAVE_BLOCKED);
 	tdb_traverse(tdb, traverse2, NULL);
 
 	/* That should *not* release the transaction lock! */
-	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+	    == WOULD_HAVE_BLOCKED);
 	return 0;
 }
 
@@ -55,7 +60,7 @@ int main(int argc, char *argv[])
 	struct tdb_context *tdb;
 	TDB_DATA key, data;
 
-	plan_tests(15);
+	plan_tests(17);
 	agent = prepare_external_agent();
 	if (!agent)
 		err(1, "preparing agent");
@@ -64,7 +69,11 @@ int main(int argc, char *argv[])
 		       O_CREAT|O_TRUNC|O_RDWR, 0600);
 	ok1(tdb);
 
-	ok1(external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, OPEN, tdb_name(tdb)) == SUCCESS);
+	ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+	    == SUCCESS);
+	ok1(external_agent_operation(agent, TRANSACTION_COMMIT, tdb_name(tdb))
+	    == SUCCESS);
 
 	key.dsize = strlen("hi");
 	key.dptr = (void *)"hi";

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

@@ -26,15 +26,11 @@ static int ftruncate_check(int fd, off_t length);
 #include <stdbool.h>
 #include <stdarg.h>
 #include <err.h>
-#include "external-transaction.h"
+#include "external-agent.h"
 
 static struct agent *agent;
-static bool agent_pending;
-static bool in_transaction;
+static bool opened;
 static int errors = 0;
-static bool snapshot_uptodate;
-static char *snapshot;
-static off_t snapshot_len;
 static bool clear_if_first;
 #define TEST_DBNAME "run-open-during-transaction.tdb"
 
@@ -57,124 +53,103 @@ static void taplog(struct tdb_context *tdb,
 	diag("%s", line);
 }
 
-static void save_file_contents(int fd)
+static bool is_same(const char *snapshot, const char *latest, off_t len)
 {
-	struct stat st;
-	int res;
+	unsigned i;
 
-	/* 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;
+	for (i = 0; i < len; i++) {
+		if (snapshot[i] != latest[i])
+			return false;
 	}
-	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;
+	return true;
+}
+
+static bool compare_file(int fd, const char *snapshot, off_t snapshot_len)
+{
+	char *contents;
+	bool same;
+
+	/* over-length read serves as length check. */
+	contents = malloc(snapshot_len+1);
+	same = pread(fd, contents, snapshot_len+1, 0) == snapshot_len
+		&& is_same(snapshot, contents, snapshot_len);
+	free(contents);
+	return same;
 }
 
-static void check_for_agent(int fd, bool block)
+static void check_file_intact(int fd)
 {
+	enum agent_return ret;
 	struct stat st;
-	int res;
+	char *contents;
 
-	if (!external_agent_operation_check(agent, block, &res))
+	fstat(fd, &st);
+	contents = malloc(st.st_size);
+	if (pread(fd, contents, st.st_size, 0) != st.st_size) {
+		diag("Read fail");
+		errors++;
 		return;
+	}
 
-	if (res != 1)
-		err(1, "Agent failed open");
-	agent_pending = false;
-
-	if (!snapshot_uptodate)
-		return;
+	/* Ask agent to open file. */
+	ret = external_agent_operation(agent, clear_if_first ?
+				       OPEN_WITH_CLEAR_IF_FIRST :
+				       OPEN,
+				       TEST_DBNAME);
 
-	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);
+	/* It's OK to open it, but it must not have changed! */
+	if (!compare_file(fd, contents, st.st_size)) {
+		diag("Agent changed file after opening %s",
+		     agent_return_name(ret));
 		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");
+	if (ret == SUCCESS) {
+		ret = external_agent_operation(agent, CLOSE, NULL);
+		if (ret != SUCCESS) {
+			diag("Agent failed to close tdb: %s",
+			     agent_return_name(ret));
+			errors++;
+		}
+	} else if (ret != WOULD_HAVE_BLOCKED) {
+		diag("Agent opening file gave %s",
+		     agent_return_name(ret));
 		errors++;
-		return;
 	}
+
+	free(contents);
 }
 
-static void check_file_contents(int fd)
+static void after_unlock(int fd)
 {
-	if (!in_transaction)
-		return;
-
-	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);
+	if (opened)
+		check_file_intact(fd);
 }
-
+	
 static ssize_t pwrite_check(int fd,
 			    const void *buf, size_t count, off_t offset)
 {
-	ssize_t ret;
-
-	check_file_contents(fd);
-
-	snapshot_uptodate = false;
-	ret = pwrite(fd, buf, count, offset);
-	if (ret != count)
-		return ret;
+	if (opened)
+		check_file_intact(fd);
 
-	check_file_contents(fd);
-	return ret;
+	return pwrite(fd, buf, count, offset);
 }
 
 static ssize_t write_check(int fd, const void *buf, size_t count)
 {
-	ssize_t ret;
+	if (opened)
+		check_file_intact(fd);
 
-	check_file_contents(fd);
-
-	snapshot_uptodate = false;
-
-	ret = write(fd, buf, count);
-	if (ret != count)
-		return ret;
-
-	check_file_contents(fd);
-	return ret;
+	return write(fd, buf, count);
 }
 
 static int ftruncate_check(int fd, off_t length)
 {
-	int ret;
+	if (opened)
+		check_file_intact(fd);
 
-	check_file_contents(fd);
+	return ftruncate(fd, length);
 
-	snapshot_uptodate = false;
-
-	ret = ftruncate(fd, length);
-
-	check_file_contents(fd);
-	return ret;
 }
 
 int main(int argc, char *argv[])
@@ -189,15 +164,11 @@ int main(int argc, char *argv[])
 	TDB_DATA key, data;
 
 	plan_tests(20);
-	unlock_callback = check_file_contents;
 	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);
-
+	unlock_callback = after_unlock;
 	for (i = 0; i < sizeof(flags)/sizeof(flags[0]); i++) {
 		clear_if_first = (flags[i] & TDB_CLEAR_IF_FIRST);
 		diag("Test with %s and %s\n",
@@ -209,8 +180,8 @@ int main(int argc, char *argv[])
 				  &logctx, NULL);
 		ok1(tdb);
 
+		opened = true;
 		ok1(tdb_transaction_start(tdb) == 0);
-		in_transaction = true;
 		key.dsize = strlen("hi");
 		key.dptr = (void *)"hi";
 		data.dptr = (void *)"world";
@@ -218,10 +189,9 @@ int main(int argc, char *argv[])
 
 		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 unexpected changes", errors);
+		ok(!errors, "We had %u open errors", errors);
 
+		opened = false;
 		tdb_close(tdb);
 	}
 

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

@@ -1,4 +1,6 @@
 #define _XOPEN_SOURCE 500
+#include "lock-tracking.h"
+#define fcntl fcntl_with_lockcheck
 #include <ccan/tdb/tdb.h>
 #include <ccan/tdb/io.c>
 #include <ccan/tdb/tdb.c>
@@ -10,10 +12,11 @@
 #include <ccan/tdb/open.c>
 #include <ccan/tdb/check.c>
 #include <ccan/tap/tap.h>
+#undef fcntl_with_lockcheck
 #include <stdlib.h>
 #include <stdbool.h>
 #include <err.h>
-#include "external-transaction.h"
+#include "external-agent.h"
 
 static struct agent *agent;
 
@@ -58,21 +61,25 @@ int main(int argc, char *argv[])
 
 	ok1(tdb_store(tdb, key, data, TDB_INSERT) == 0);
 
-	ok1(external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, OPEN, tdb_name(tdb)) == SUCCESS);
 
 	ok1(tdb_transaction_start(tdb) == 0);
-	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+	    == WOULD_HAVE_BLOCKED);
 	tdb_traverse(tdb, traverse, NULL);
 
 	/* That should *not* release the transaction lock! */
-	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+	    == WOULD_HAVE_BLOCKED);
 	tdb_traverse_read(tdb, traverse, NULL);
 
 	/* That should *not* release the transaction lock! */
-	ok1(!external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+	    == WOULD_HAVE_BLOCKED);
 	ok1(tdb_transaction_commit(tdb) == 0);
 	/* Now we should be fine. */
-	ok1(external_agent_operation(agent, TRANSACTION, tdb_name(tdb)));
+	ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+	    == SUCCESS);
 
 	tdb_close(tdb);