From: Rusty Russell Date: Tue, 2 Feb 2010 01:22:19 +0000 (+1030) Subject: New test: reveals race (found by Volker) when open occurs during transaction commit. X-Git-Url: http://git.ozlabs.org/?p=ccan;a=commitdiff_plain;h=02b364ae6c3a120101aa1fd6eb0919994ca8ffb1 New test: reveals race (found by Volker) when open occurs during transaction commit. --- diff --git a/ccan/tdb/test/external-transaction.c b/ccan/tdb/test/external-transaction.c index c03dbc49..1211da9e 100644 --- a/ccan/tdb/test/external-transaction.c +++ b/ccan/tdb/test/external-transaction.c @@ -7,7 +7,9 @@ #include #include #include +#include #include +#include 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; +} diff --git a/ccan/tdb/test/external-transaction.h b/ccan/tdb/test/external-transaction.h index 62a4803b..843b6979 100644 --- a/ccan/tdb/test/external-transaction.h +++ b/ccan/tdb/test/external-transaction.h @@ -2,10 +2,24 @@ #define TDB_TEST_EXTERNAL_TRANSACTION_H #include +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 */ diff --git a/ccan/tdb/test/run-nested-traverse.c b/ccan/tdb/test/run-nested-traverse.c index e78a14ef..4470610a 100644 --- a/ccan/tdb/test/run-nested-traverse.c +++ b/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"; diff --git a/ccan/tdb/test/run-open-during-transaction.c b/ccan/tdb/test/run-open-during-transaction.c new file mode 100644 index 00000000..b0eed67b --- /dev/null +++ b/ccan/tdb/test/run-open-during-transaction.c @@ -0,0 +1,239 @@ +#define _XOPEN_SOURCE 500 +#include +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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#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(); +} diff --git a/ccan/tdb/test/run-traverse-in-transaction.c b/ccan/tdb/test/run-traverse-in-transaction.c index 5b1c870c..37a4f6d3 100644 --- a/ccan/tdb/test/run-traverse-in-transaction.c +++ b/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);