From: Rusty Russell Date: Tue, 29 Nov 2011 22:34:11 +0000 (+1030) Subject: failtest: use a linked list for history, not an array. X-Git-Url: http://git.ozlabs.org/?p=ccan;a=commitdiff_plain;h=9571a41e8494f3135557e3ec50c2de856392173e failtest: use a linked list for history, not an array. This avoids a silly realloc, but more importantly it gets us closer to being runtime extensible, as each history element can be a different size. --- diff --git a/ccan/failtest/_info b/ccan/failtest/_info index a9189a23..d8b2a252 100644 --- a/ccan/failtest/_info +++ b/ccan/failtest/_info @@ -61,10 +61,12 @@ int main(int argc, char *argv[]) return 1; if (strcmp(argv[1], "depends") == 0) { + printf("ccan/build_assert\n"); printf("ccan/compiler\n"); printf("ccan/read_write_all\n"); - printf("ccan/build_assert\n"); + printf("ccan/str\n"); printf("ccan/time\n"); + printf("ccan/tlist\n"); return 0; } diff --git a/ccan/failtest/failtest.c b/ccan/failtest/failtest.c index c95a94f8..f59ed03a 100644 --- a/ccan/failtest/failtest.c +++ b/ccan/failtest/failtest.c @@ -19,8 +19,9 @@ #include #include #include +#include -enum failtest_result (*failtest_hook)(struct failtest_call *, unsigned); +enum failtest_result (*failtest_hook)(struct tlist_calls *); static int tracefd = -1; @@ -44,10 +45,9 @@ struct lock_info { int type; }; -bool (*failtest_exit_check)(struct failtest_call *history, unsigned num); +bool (*failtest_exit_check)(struct tlist_calls *history); -static struct failtest_call *history = NULL; -static unsigned int history_num = 0; +static struct tlist_calls history = TLIST_INIT(history); static int control_fd = -1; static struct timeval start; static unsigned int probe_count = 0; @@ -72,17 +72,20 @@ static struct failtest_call *add_history_(enum failtest_call_type type, const void *elem, size_t elem_size) { + struct failtest_call *call; + /* NULL file is how we suppress failure. */ if (!file) return &unrecorded_call; - history = realloc(history, (history_num + 1) * sizeof(*history)); - history[history_num].type = type; - history[history_num].file = file; - history[history_num].line = line; - history[history_num].cleanup = NULL; - memcpy(&history[history_num].u, elem, elem_size); - return &history[history_num++]; + call = malloc(sizeof *call); + call->type = type; + call->file = file; + call->line = line; + call->cleanup = NULL; + memcpy(&call->u, elem, elem_size); + tlist_add_tail(&history, call, list); + return call; } #define add_history(type, file, line, elem) \ @@ -114,15 +117,18 @@ static bool read_write_info(int fd) static char *failpath_string(void) { - unsigned int i; - char *ret = malloc(history_num + 1); - - for (i = 0; i < history_num; i++) { - ret[i] = info_to_arg[history[i].type]; - if (history[i].fail) - ret[i] = toupper(ret[i]); + struct failtest_call *i; + char *ret = strdup(""); + unsigned len = 0; + + /* Inefficient, but who cares? */ + tlist_for_each(&history, i, list) { + ret = realloc(ret, len + 2); + ret[len] = info_to_arg[i->type]; + if (i->fail) + ret[len] = toupper(ret[len]); + ret[++len] = '\0'; } - ret[i] = '\0'; return ret; } @@ -265,23 +271,23 @@ static struct saved_file *save_file(struct saved_file *next, int fd) static struct saved_file *save_files(void) { struct saved_file *files = NULL; - int i; + struct failtest_call *i; /* Figure out the set of live fds. */ - for (i = history_num - 2; i >= 0; i--) { - if (history[i].type == FAILTEST_OPEN) { - int fd = history[i].u.open.ret; + tlist_for_each_rev(&history, i, list) { + if (i->type == FAILTEST_OPEN) { + int fd = i->u.open.ret; /* Only do successful, writable fds. */ if (fd < 0) continue; /* If it was closed, cleanup == NULL. */ - if (!history[i].cleanup) + if (!i->cleanup) continue; - if ((history[i].u.open.flags & O_RDWR) == O_RDWR) { + if ((i->u.open.flags & O_RDWR) == O_RDWR) { files = save_file(files, fd); - } else if ((history[i].u.open.flags & O_WRONLY) + } else if ((i->u.open.flags & O_WRONLY) == O_WRONLY) { /* FIXME: Handle O_WRONLY. Open with O_RDWR? */ abort(); @@ -320,41 +326,48 @@ static void free_files(struct saved_file *s) } } +static void free_call(struct failtest_call *call) +{ + /* We don't do this in cleanup: needed even for failed opens. */ + if (call->type == FAILTEST_OPEN) + free((char *)call->u.open.pathname); + tlist_del_from(&history, call, list); + free(call); +} + /* Free up memory, so valgrind doesn't report leaks. */ static void free_everything(void) { - unsigned int i; + struct failtest_call *i; - /* We don't do this in cleanup: needed even for failed opens. */ - for (i = 0; i < history_num; i++) { - if (history[i].type == FAILTEST_OPEN) - free((char *)history[i].u.open.pathname); - } - free(history); + while ((i = tlist_top(&history, struct failtest_call, list)) != NULL) + free_call(i); } static NORETURN void failtest_cleanup(bool forced_cleanup, int status) { - int i; + struct failtest_call *i; /* For children, we don't care if they "failed" the testing. */ if (control_fd != -1) status = 0; - if (forced_cleanup) - history_num--; + if (forced_cleanup) { + /* We didn't actually do final operation: remove it. */ + i = tlist_tail(&history, struct failtest_call, list); + free_call(i); + } /* Cleanup everything, in reverse order. */ - for (i = history_num - 1; i >= 0; i--) { - if (!history[i].cleanup) + tlist_for_each_rev(&history, i, list) { + if (!i->cleanup) continue; if (!forced_cleanup) { printf("Leak at %s:%u: --failpath=%s\n", - history[i].file, history[i].line, - failpath_string()); + i->file, i->line, failpath_string()); status = 1; } - history[i].cleanup(&history[i].u); + i->cleanup(&i->u); } free_everything(); @@ -390,23 +403,16 @@ static bool should_fail(struct failtest_call *call) != info_to_arg[call->type]) errx(1, "Failpath expected '%c' got '%c'\n", info_to_arg[call->type], *failpath); - call->fail = isupper((unsigned char)*(failpath++)); + call->fail = cisupper(*(failpath++)); return call->fail; } } /* Attach debugger if they asked for it. */ - if (debugpath && history_num == strlen(debugpath)) { - unsigned int i; + if (debugpath) { + char *path = failpath_string(); - for (i = 0; i < history_num; i++) { - unsigned char c = info_to_arg[history[i].type]; - if (history[i].fail) - c = toupper(c); - if (c != debugpath[i]) - break; - } - if (i == history_num) { + if (streq(path, debugpath)) { char str[80]; /* Don't timeout. */ @@ -415,11 +421,15 @@ static bool should_fail(struct failtest_call *call) getpid(), getpid()); if (system(str) == 0) sleep(5); + } else if (!strstarts(path, debugpath)) { + fprintf(stderr, "--debugpath not followed: %s\n", path); + debugpath = NULL; } + free(path); } if (failtest_hook) { - switch (failtest_hook(history, history_num)) { + switch (failtest_hook(&history)) { case FAIL_OK: break; case FAIL_PROBE: @@ -460,18 +470,22 @@ static bool should_fail(struct failtest_call *call) if (tracefd != -1) { struct timeval diff; const char *p; + char *failpath; + struct failtest_call *c; + c = tlist_tail(&history, struct failtest_call, list); diff = time_sub(time_now(), start); - p = failpath_string(); + failpath = failpath_string(); trace("%u->%u (%u.%02u): %s (", getppid(), getpid(), - (int)diff.tv_sec, (int)diff.tv_usec / 10000, p); - free((char *)p); - p = strrchr(history[history_num-1].file, '/'); + (int)diff.tv_sec, (int)diff.tv_usec / 10000, + failpath); + free(failpath); + p = strrchr(c->file, '/'); if (p) trace("%s", p+1); else - trace("%s", history[history_num-1].file); - trace(":%u)\n", history[history_num-1].line); + trace("%s", c->file); + trace(":%u)\n", c->line); } close(control[0]); close(output[0]); @@ -619,28 +633,28 @@ static void cleanup_realloc(struct realloc_call *call) } /* Walk back and find out if we got this ptr from a previous routine. */ -static void fixup_ptr_history(void *ptr, unsigned int last) +static void fixup_ptr_history(void *ptr) { - int i; + struct failtest_call *i; /* Start at end of history, work back. */ - for (i = last - 1; i >= 0; i--) { - switch (history[i].type) { + tlist_for_each_rev(&history, i, list) { + switch (i->type) { case FAILTEST_REALLOC: - if (history[i].u.realloc.ret == ptr) { - history[i].cleanup = NULL; + if (i->u.realloc.ret == ptr) { + i->cleanup = NULL; return; } break; case FAILTEST_MALLOC: - if (history[i].u.malloc.ret == ptr) { - history[i].cleanup = NULL; + if (i->u.malloc.ret == ptr) { + i->cleanup = NULL; return; } break; case FAILTEST_CALLOC: - if (history[i].u.calloc.ret == ptr) { - history[i].cleanup = NULL; + if (i->u.calloc.ret == ptr) { + i->cleanup = NULL; return; } break; @@ -662,7 +676,9 @@ void *failtest_realloc(void *ptr, size_t size, const char *file, unsigned line) p->u.realloc.ret = NULL; p->error = ENOMEM; } else { - fixup_ptr_history(ptr, history_num-1); + /* Don't catch this one in the history fixup... */ + p->u.realloc.ret = NULL; + fixup_ptr_history(ptr); p->u.realloc.ret = realloc(ptr, size); set_cleanup(p, cleanup_realloc, struct realloc_call); } @@ -672,7 +688,7 @@ void *failtest_realloc(void *ptr, size_t size, const char *file, unsigned line) void failtest_free(void *ptr) { - fixup_ptr_history(ptr, history_num); + fixup_ptr_history(ptr); free(ptr); } @@ -910,7 +926,7 @@ add_lock(struct lock_info *locks, int fd, off_t start, off_t end, int type) /* We trap this so we can record it: we don't fail it. */ int failtest_close(int fd, const char *file, unsigned line) { - int i; + struct failtest_call *i; struct close_call call; struct failtest_call *p; @@ -927,30 +943,30 @@ int failtest_close(int fd, const char *file, unsigned line) return close(fd); /* Trace history to find source of fd. */ - for (i = history_num-1; i >= 0; i--) { - switch (history[i].type) { + tlist_for_each_rev(&history, i, list) { + switch (i->type) { case FAILTEST_PIPE: /* From a pipe? */ - if (history[i].u.pipe.fds[0] == fd) { - assert(!history[i].u.pipe.closed[0]); - history[i].u.pipe.closed[0] = true; - if (history[i].u.pipe.closed[1]) - history[i].cleanup = NULL; + if (i->u.pipe.fds[0] == fd) { + assert(!i->u.pipe.closed[0]); + i->u.pipe.closed[0] = true; + if (i->u.pipe.closed[1]) + i->cleanup = NULL; goto out; } - if (history[i].u.pipe.fds[1] == fd) { - assert(!history[i].u.pipe.closed[1]); - history[i].u.pipe.closed[1] = true; - if (history[i].u.pipe.closed[0]) - history[i].cleanup = NULL; + if (i->u.pipe.fds[1] == fd) { + assert(!i->u.pipe.closed[1]); + i->u.pipe.closed[1] = true; + if (i->u.pipe.closed[0]) + i->cleanup = NULL; goto out; } break; case FAILTEST_OPEN: - if (history[i].u.open.ret == fd) { - assert((void *)history[i].cleanup + if (i->u.open.ret == fd) { + assert((void *)i->cleanup == (void *)cleanup_open); - history[i].cleanup = NULL; + i->cleanup = NULL; goto out; } break; @@ -1074,7 +1090,7 @@ bool failtest_has_failed(void) void failtest_exit(int status) { if (failtest_exit_check) { - if (!failtest_exit_check(history, history_num)) + if (!failtest_exit_check(&history)) child_fail(NULL, 0, "failtest_exit_check failed\n"); } diff --git a/ccan/failtest/failtest.h b/ccan/failtest/failtest.h index 0cc8f60c..294c28b9 100644 --- a/ccan/failtest/failtest.h +++ b/ccan/failtest/failtest.h @@ -9,6 +9,7 @@ #include #include #include +#include /** * failtest_init - initialize the failtest module @@ -124,6 +125,8 @@ struct fcntl_call { * failtest_hook, failtest_exit_check */ struct failtest_call { + /* We're in the history list. */ + struct list_node list; enum failtest_call_type type; /* Where we were called from. */ const char *file; @@ -148,6 +151,9 @@ struct failtest_call { } u; }; +/* This defines struct tlist_calls. */ +TLIST_TYPE(calls, struct failtest_call); + enum failtest_result { /* Yes try failing this call. */ FAIL_OK, @@ -160,7 +166,6 @@ enum failtest_result { /** * failtest_hook - whether a certain call should fail or not. * @history: the ordered history of all failtest calls. - * @num: the number of elements in @history (greater than 0) * * The default value of this hook is failtest_default_hook(), which returns * FAIL_OK (ie. yes, fail the call). @@ -170,25 +175,24 @@ enum failtest_result { * call. * * Example: - * static enum failtest_result dont_fail_alloc(struct failtest_call *hist, - * unsigned num) + * static enum failtest_result dont_fail_alloc(struct tlist_calls *history) * { - * if (hist[num-1].type == FAILTEST_MALLOC - * || hist[num-1].type == FAILTEST_CALLOC - * || hist[num-1].type == FAILTEST_REALLOC) + * struct failtest_call *call; + * call = tlist_tail(history, struct failtest_call, list); + * if (call->type == FAILTEST_MALLOC + * || call->type == FAILTEST_CALLOC + * || call->type == FAILTEST_REALLOC) * return FAIL_DONT_FAIL; * return FAIL_OK; * } * ... * failtest_hook = dont_fail_alloc; */ -extern enum failtest_result -(*failtest_hook)(struct failtest_call *history, unsigned num); +extern enum failtest_result (*failtest_hook)(struct tlist_calls *history); /** * failtest_exit_check - hook for additional checks on a failed child. * @history: the ordered history of all failtest calls. - * @num: the number of elements in @history (greater than 0) * * Your program might have additional checks to do on failure, such as * check that a file is not corrupted, or than an error message has been @@ -197,16 +201,15 @@ extern enum failtest_result * If this returns false, the path to this failure will be printed and the * overall test will fail. */ -extern bool (*failtest_exit_check)(struct failtest_call *history, - unsigned num); +extern bool (*failtest_exit_check)(struct tlist_calls *history); /** * failtest_has_failed - determine if a failure has occurred. * - * Sometimes you want to exit immediately if you've experienced a failure. - * This is useful when you have four separate tests in your test suite, - * and you don't want to do the next one if you've had a failure in a - * previous one. + * Sometimes you want to exit immediately if you've experienced an + * injected failure. This is useful when you have four separate tests + * in your test suite, and you don't want to do the next one if you've + * had a failure in a previous one. */ extern bool failtest_has_failed(void); diff --git a/ccan/failtest/test/run-history.c b/ccan/failtest/test/run-history.c index ef28a3f5..24dc1169 100644 --- a/ccan/failtest/test/run-history.c +++ b/ccan/failtest/test/run-history.c @@ -102,20 +102,24 @@ int main(void) ok1(call->u.write.fd == write_call.fd); ok1(call->u.write.count == write_call.count); - ok1(history_num == 7); + i = 0; + tlist_for_each(&history, call, list) + i++; - for (i = 0; i < history_num; i++) - history[i].fail = false; + ok1(i == 7); + + tlist_for_each(&history, call, list) + call->fail = false; path = failpath_string(); - ok1(strcmp(path, "cmeoprw") == 0); + ok1(streq(path, "cmeoprw")); free(path); - for (i = 0; i < history_num; i++) - history[i].fail = true; + tlist_for_each(&history, call, list) + call->fail = true; path = failpath_string(); - ok1(strcmp(path, "CMEOPRW") == 0); + ok1(streq(path, "CMEOPRW")); free(path); return exit_status(); diff --git a/ccan/failtest/test/run-locking.c b/ccan/failtest/test/run-locking.c index 6ebcd6ea..f29f213d 100644 --- a/ccan/failtest/test/run-locking.c +++ b/ccan/failtest/test/run-locking.c @@ -10,8 +10,7 @@ #define SIZE 8 /* We don't want to fork and fail; we're just testing lock recording. */ -static enum failtest_result dont_fail(struct failtest_call *history, - unsigned num) +static enum failtest_result dont_fail(struct tlist_calls *history) { return FAIL_DONT_FAIL; } diff --git a/ccan/tdb2/test/failtest_helper.c b/ccan/tdb2/test/failtest_helper.c index d24ac4c4..f3ef09a6 100644 --- a/ccan/tdb2/test/failtest_helper.c +++ b/ccan/tdb2/test/failtest_helper.c @@ -26,12 +26,14 @@ bool failmatch(const struct failtest_call *call, } static const struct failtest_call * -find_repeat(const struct failtest_call *start, const struct failtest_call *end, +find_repeat(const struct tlist_calls *history, const struct failtest_call *call) { const struct failtest_call *i; - for (i = start; i < end; i++) { + tlist_for_each(history, i, list) { + if (i != call) + continue; if (failmatch(i, call->file, call->line, call->type)) return i; } @@ -49,32 +51,31 @@ static bool is_unlock(const struct failtest_call *call) && call->u.fcntl.arg.fl.l_type == F_UNLCK; } -bool exit_check_log(struct failtest_call *history, unsigned num) +bool exit_check_log(struct tlist_calls *history) { - unsigned int i; + const struct failtest_call *i; - for (i = 0; i < num; i++) { - if (!history[i].fail) + tlist_for_each(history, i, list) { + if (!i->fail) continue; /* Failing the /dev/urandom open doesn't count: we fall back. */ - if (failmatch(&history[i], URANDOM_OPEN)) + if (failmatch(i, URANDOM_OPEN)) continue; /* Similarly with read fail. */ - if (failmatch(&history[i], URANDOM_READ)) + if (failmatch(i, URANDOM_READ)) continue; /* Initial allocation of tdb doesn't log. */ - if (failmatch(&history[i], INITIAL_TDB_MALLOC)) + if (failmatch(i, INITIAL_TDB_MALLOC)) continue; /* We don't block "failures" on non-blocking locks. */ - if (is_nonblocking_lock(&history[i])) + if (is_nonblocking_lock(i)) continue; if (!tap_log_messages) - diag("We didn't log for %u (%s:%u)", - i, history[i].file, history[i].line); + diag("We didn't log for %s:%u", i->file, i->line); return tap_log_messages != 0; } return true; @@ -82,9 +83,11 @@ bool exit_check_log(struct failtest_call *history, unsigned num) /* Some places we soldier on despite errors: only fail them once. */ enum failtest_result -block_repeat_failures(struct failtest_call *history, unsigned num) +block_repeat_failures(struct tlist_calls *history) { - const struct failtest_call *i, *last = &history[num-1]; + const struct failtest_call *i, *last; + + last = tlist_tail(history, struct failtest_call, list); if (failtest_suppress) return FAIL_DONT_FAIL; @@ -92,7 +95,7 @@ block_repeat_failures(struct failtest_call *history, unsigned num) if (failmatch(last, INITIAL_TDB_MALLOC) || failmatch(last, URANDOM_OPEN) || failmatch(last, URANDOM_READ)) { - if (find_repeat(history, last, last)) + if (find_repeat(history, last)) return FAIL_DONT_FAIL; return FAIL_PROBE; } @@ -100,21 +103,15 @@ block_repeat_failures(struct failtest_call *history, unsigned num) /* Unlock or non-blocking lock is fail-once. */ if (is_unlock(last)) { /* Find a previous unlock at this point? */ - for (i = find_repeat(history, last, last); - i; - i = find_repeat(history, i, last)) { - if (is_unlock(i)) - return FAIL_DONT_FAIL; - } + i = find_repeat(history, last); + if (i && is_unlock(i)) + return FAIL_DONT_FAIL; return FAIL_PROBE; } else if (is_nonblocking_lock(last)) { /* Find a previous non-blocking lock at this point? */ - for (i = find_repeat(history, last, last); - i; - i = find_repeat(history, i, last)) { - if (is_nonblocking_lock(i)) - return FAIL_DONT_FAIL; - } + i = find_repeat(history, last); + if (i && is_nonblocking_lock(i)) + return FAIL_DONT_FAIL; return FAIL_PROBE; } diff --git a/ccan/tdb2/test/failtest_helper.h b/ccan/tdb2/test/failtest_helper.h index 7f912bb5..a3c68088 100644 --- a/ccan/tdb2/test/failtest_helper.h +++ b/ccan/tdb2/test/failtest_helper.h @@ -8,11 +8,10 @@ #define URANDOM_OPEN "open.c", 62, FAILTEST_OPEN #define URANDOM_READ "open.c", 42, FAILTEST_READ -bool exit_check_log(struct failtest_call *history, unsigned num); +bool exit_check_log(struct tlist_calls *history); bool failmatch(const struct failtest_call *call, const char *file, int line, enum failtest_call_type type); -enum failtest_result -block_repeat_failures(struct failtest_call *history, unsigned num); +enum failtest_result block_repeat_failures(struct tlist_calls *history); /* Set this to suppress failure. */ extern bool failtest_suppress;