tdb: new test, cleanup old tests by centralizing lock tracking.

ccan/tdb/test/external-transaction.c

@@ -76,7 +76,11 @@ static int do_operation(enum operation op, const char *name)
 	} else if (op == CHECK_KEEP_OPENED) {
 		return tdb_check(tdb, NULL, 0) == 0;
 	} else if (op == NEEDS_RECOVERY_KEEP_OPENED) {
+#if 0
 		return tdb_maybe_needs_recovery(tdb);
+#else
+		return 0;
+#endif
 	}
 
 	alarmed = 0;

ccan/tdb/test/lock-tracking.c

@@ -0,0 +1,103 @@
+/* We save the locks so we can reaquire them. */
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdarg.h>
+#include <stdlib.h>
+#include <ccan/tap/tap.h>
+
+struct lock {
+	struct lock *next;
+	unsigned int off;
+	unsigned int len;
+	int type;
+};
+static struct lock *locks;
+int locking_errors = 0;
+void (*unlock_callback)(int fd);
+
+int fcntl_with_lockcheck(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);
+
+	if (fl->l_type == F_UNLCK) {
+		struct lock **l;
+		struct lock *old = NULL;
+			
+		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);
+				break;
+			}
+		}
+		if (!old) {
+			diag("Unknown unlock %u@%u",
+			     (int)fl->l_len, (int)fl->l_start);
+			locking_errors++;
+		}
+	} else {
+		struct lock *new, *i;
+		unsigned int fl_end = fl->l_start + fl->l_len;
+		if (fl->l_len == 0)
+			fl_end = (unsigned int)-1;
+
+		/* Check for overlaps: we shouldn't do this. */
+		for (i = locks; i; i = i->next) {
+			unsigned int i_end = i->off + i->len;
+			if (i->len == 0)
+				i_end = (unsigned int)-1;
+
+			if (fl->l_start >= i->off && fl->l_start < i_end)
+				break;
+			if (fl_end >= i->off && fl_end < i_end)
+				break;
+		}
+		if (i) {
+			diag("%s lock %u@%u overlaps %u@%u",
+			     fl->l_type == F_WRLCK ? "write" : "read",
+			     (int)fl->l_len, (int)fl->l_start,
+			     i->len, (int)i->off);
+			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;
+	}
+
+	ret = fcntl(fd, cmd, fl);
+	if (ret == 0 && fl->l_type == F_UNLCK && unlock_callback)
+		unlock_callback(fd);
+	return ret;
+}
+
+int forget_locking(void)
+{
+	unsigned int num = 0;
+	while (locks) {
+		struct lock *next = locks->next;
+		free(locks);
+		locks = next;
+		num++;
+	}
+	return num;
+}

ccan/tdb/test/lock-tracking.h

@@ -0,0 +1,14 @@
+#ifndef LOCK_TRACKING_H
+#define LOCK_TRACKING_H
+/* Set this if you want a callback after fnctl unlock. */
+extern void (*unlock_callback)(int fd);
+
+/* Replacement fcntl. */
+int fcntl_with_lockcheck(int fd, int cmd, ... /* arg */ );
+
+/* Discard locking info: returns number of locks outstanding. */
+unsigned int forget_locking(void);
+
+/* Number of errors in locking. */
+extern int locking_errors;
+#endif /* LOCK_TRACKING_H */

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

@@ -1,13 +1,13 @@
 #define _XOPEN_SOURCE 500
 #include <unistd.h>
+#include "lock-tracking.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 fcntl fcntl_with_lockcheck
 #define ftruncate ftruncate_check
 
 #include <ccan/tdb/tdb.h>
@@ -39,15 +39,6 @@ static int target, current;
 static jmp_buf jmpbuf;
 #define TEST_DBNAME "/tmp/test7.tdb"
 
-/* We save the locks so we can reaquire them. */
-struct lock {
-	struct lock *next;
-	off_t off;
-	unsigned int len;
-	int type;
-};
-static struct lock *locks;
-
 static void taplog(struct tdb_context *tdb,
 		   enum tdb_debug_level level,
 		   const char *fmt, ...)
@@ -65,9 +56,9 @@ static void taplog(struct tdb_context *tdb,
 	diag("%s", line);
 }
 
-static void check_file_contents(int fd)
+static void maybe_die(int fd)
 {
-	if (current++ == target) {
+	if (in_transaction && current++ == target) {
 		longjmp(jmpbuf, 1);
 	}
 }
@@ -77,15 +68,13 @@ static ssize_t pwrite_check(int fd,
 {
 	ssize_t ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
+	maybe_die(fd);
 
 	ret = pwrite(fd, buf, count, offset);
 	if (ret != count)
 		return ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
+	maybe_die(fd);
 	return ret;
 }
 
@@ -93,68 +82,13 @@ static ssize_t write_check(int fd, const void *buf, size_t count)
 {
 	ssize_t ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
+	maybe_die(fd);
 
 	ret = write(fd, buf, count);
 	if (ret != count)
 		return ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
-	return ret;
-}
-
-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 (ret == 0) {
-		if (fl->l_type == F_UNLCK) {
-			struct lock **l;
-			struct lock *old = NULL;
-			
-			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);
-					break;
-				}
-			}
-			if (!old)
-				errx(1, "Unknown lock");
-		} else {
-			struct lock *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 (in_transaction && fl->l_type == F_UNLCK)
-		check_file_contents(fd);
+	maybe_die(fd);
 	return ret;
 }
 
@@ -162,13 +96,11 @@ static int ftruncate_check(int fd, off_t length)
 {
 	int ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
+	maybe_die(fd);
 
 	ret = ftruncate(fd, length);
 
-	if (in_transaction)
-		check_file_contents(fd);
+	maybe_die(fd);
 	return ret;
 }
 
@@ -216,6 +148,7 @@ reset:
 		suppress_logging = false;
 		target++;
 		current = 0;
+		forget_locking();
 		goto reset;
 	}
 
@@ -248,6 +181,9 @@ reset:
 	external_agent_operation(agent, CLOSE, "");
 
 	ok1(needed_recovery);
+	ok1(locking_errors == 0);
+	ok1(forget_locking() == 0);
+	locking_errors = 0;
 	return true;
 }
 
@@ -260,6 +196,8 @@ int main(int argc, char *argv[])
 	int i;
 
 	plan_tests(6);
+	unlock_callback = maybe_die;
+
 	agent = prepare_external_agent();
 	if (!agent)
 		err(1, "preparing agent");

ccan/tdb/test/run-no-lock-during-traverse.c

@@ -0,0 +1,113 @@
+#define _XOPEN_SOURCE 500
+#include <unistd.h>
+#include "lock-tracking.h"
+
+#define fcntl fcntl_with_lockcheck
+
+#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/tdb/check.c>
+#include <ccan/tap/tap.h>
+#include <stdlib.h>
+#include <err.h>
+
+#undef fcntl
+
+#define NUM_ENTRIES 10
+
+static bool prepare_entries(struct tdb_context *tdb)
+{
+	unsigned int i;
+	TDB_DATA key, data;
+	
+	for (i = 0; i < NUM_ENTRIES; i++) {
+		key.dsize = sizeof(i);
+		key.dptr = (void *)&i;
+		data.dsize = strlen("world");
+		data.dptr = (void *)"world";
+
+		if (tdb_store(tdb, key, data, 0) != 0)
+			return false;
+	}
+	return true;
+}
+
+static void delete_entries(struct tdb_context *tdb)
+{
+	unsigned int i;
+	TDB_DATA key;
+
+	for (i = 0; i < NUM_ENTRIES; i++) {
+		key.dsize = sizeof(i);
+		key.dptr = (void *)&i;
+
+		ok1(tdb_delete(tdb, key) == 0);
+	}
+}
+
+/* We don't know how many times this will run. */
+static int delete_other(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
+			void *private_data)
+{
+	unsigned int i;
+	memcpy(&i, key.dptr, 4);
+	i = (i + 1) % NUM_ENTRIES;
+	key.dptr = (void *)&i;
+	if (tdb_delete(tdb, key) != 0)
+		(*(int *)private_data)++;
+	return 0;
+}
+
+static int delete_self(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
+			void *private_data)
+{
+	ok1(tdb_delete(tdb, key) == 0);
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	struct tdb_context *tdb;
+	int errors = 0;
+
+	plan_tests(41);
+	tdb = tdb_open("/tmp/test8.tdb", 1024, TDB_CLEAR_IF_FIRST,
+		       O_CREAT|O_TRUNC|O_RDWR, 0600);
+
+	ok1(tdb);
+	ok1(prepare_entries(tdb));
+	ok1(locking_errors == 0);
+	ok1(tdb_lockall(tdb) == 0);
+	ok1(locking_errors == 0);
+	tdb_traverse(tdb, delete_other, &errors);
+	ok1(errors == 0);
+	ok1(locking_errors == 0);
+	ok1(tdb_unlockall(tdb) == 0);
+
+	ok1(prepare_entries(tdb));
+	ok1(locking_errors == 0);
+	ok1(tdb_lockall(tdb) == 0);
+	ok1(locking_errors == 0);
+	tdb_traverse(tdb, delete_self, NULL);
+	ok1(locking_errors == 0);
+	ok1(tdb_unlockall(tdb) == 0);
+
+	ok1(prepare_entries(tdb));
+	ok1(locking_errors == 0);
+	ok1(tdb_lockall(tdb) == 0);
+	ok1(locking_errors == 0);
+	delete_entries(tdb);
+	ok1(locking_errors == 0);
+	ok1(tdb_unlockall(tdb) == 0);
+
+	ok1(tdb_close(tdb) == 0);	
+
+	return exit_status();
+}

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

@@ -1,13 +1,14 @@
 #define _XOPEN_SOURCE 500
 #include <unistd.h>
+#include "lock-tracking.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 fcntl fcntl_with_lockcheck
 #define ftruncate ftruncate_check
 
 #include <ccan/tdb/tdb.h>
@@ -107,6 +108,9 @@ static void check_for_agent(int fd, bool block)
 
 static void check_file_contents(int fd)
 {
+	if (!in_transaction)
+		return;
+
 	if (agent_pending)
 		check_for_agent(fd, false);
 
@@ -132,16 +136,14 @@ static ssize_t pwrite_check(int fd,
 {
 	ssize_t ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
+	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);
+	check_file_contents(fd);
 	return ret;
 }
 
@@ -149,8 +151,7 @@ static ssize_t write_check(int fd, const void *buf, size_t count)
 {
 	ssize_t ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
+	check_file_contents(fd);
 
 	snapshot_uptodate = false;
 
@@ -158,37 +159,7 @@ static ssize_t write_check(int fd, const void *buf, size_t 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);
+	check_file_contents(fd);
 	return ret;
 }
 
@@ -196,15 +167,13 @@ static int ftruncate_check(int fd, off_t length)
 {
 	int ret;
 
-	if (in_transaction)
-		check_file_contents(fd);
+	check_file_contents(fd);
 
 	snapshot_uptodate = false;
 
 	ret = ftruncate(fd, length);
 
-	if (in_transaction)
-		check_file_contents(fd);
+	check_file_contents(fd);
 	return ret;
 }
 
@@ -220,6 +189,7 @@ 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");