From c511e1f925da3e735fb494d522ae3ae0f17ab9ce Mon Sep 17 00:00:00 2001 Message-Id: From: Mark Wooding Date: Mon, 1 Jan 2007 12:52:32 +0000 Subject: [PATCH] admin: Remove locking; new safe client destruction. Organization: Straylight/Edgeware From: Mark Wooding The locking stuff is fiddly, and prevents cleanup of pending background operations during destruction. Instead, we add `destroyed' clients to a list which we clean up just before going back to select. This ensures that they get cleaned up at a safe place, when there aren't functions threaded on the stack which will be upset by the admin block vanishing under their feet. --- server/admin.c | 134 ++++++++++++++++++++++--------------------------- server/tripe.c | 1 + server/tripe.h | 13 +++++ 3 files changed, 73 insertions(+), 75 deletions(-) diff --git a/server/admin.c b/server/admin.c index b166b853..ca5dc8cf 100644 --- a/server/admin.c +++ b/server/admin.c @@ -62,6 +62,7 @@ static const trace_opt w_opts[] = { /*----- Static variables --------------------------------------------------*/ static admin *admins; +static admin *a_dead; static sel_file sock; static const char *sockname; static unsigned flags = 0; @@ -75,8 +76,6 @@ static sig s_term, s_int, s_hup; #define T_PING SEC(5) static void a_destroy(admin */*a*/); -static void a_lock(admin */*a*/); -static void a_unlock(admin */*a*/); #define BOOL(x) ((x) ? "t" : "nil") @@ -643,7 +642,6 @@ static void a_bgrelease(admin_bgop *bg) else a->bg = bg->next; xfree(bg); if (a->f & AF_CLOSE) a_destroy(a); - a_unlock(a); } /* --- @a_bgok@, @a_bginfo@, @a_bgfail@ --- * @@ -703,7 +701,6 @@ static void a_bgadd(admin *a, admin_bgop *bg, const char *tag, bg->prev = 0; if (a->bg) a->bg->prev = bg; a->bg = bg; - a_lock(a); T( trace(T_ADMIN, "admin: add bgop %s", BGTAG(bg)); ) if (tag) a_write(a, "DETACH", tag, A_END); } @@ -1379,7 +1376,6 @@ static void acmd_quit(admin *a, unsigned ac, char *av[]) { a_warn("SERVER", "quit", "admin-request", A_END); a_ok(a); - a_unlock(a); a_quit(); } @@ -1454,70 +1450,55 @@ static void acmd_help(admin *a, unsigned ac, char *av[]) /*----- Connection handling -----------------------------------------------*/ -/* --- @a_lock@ --- * +/* --- @a_destroypending@ --- * * - * Arguments: @admin *a@ = pointer to an admin block - * - * Returns: --- - * - * Use: Locks an admin block so that it won't be destroyed - * immediately. - */ - -static void a_lock(admin *a) { a->ref++; } - -/* --- @a_dodestroy@ --- * - * - * Arguments: @admin *a@ = pointer to an admin block + * Arguments: --- * * Returns: --- * - * Use: Actually does the legwork of destroying an admin block. + * Use: Destroys pending admin connections at a safe time. */ -static void a_dodestroy(admin *a) +static void a_destroypending(void) { + admin *a, *aa; admin_bgop *bg, *bbg; - T( trace(T_ADMIN, "admin: completing destruction of connection %u", - a->seq); ) + /* --- Destroy connections marked as pending --- */ - selbuf_destroy(&a->b); - for (bg = a->bg; bg; bg = bbg) { - bbg = bg->next; - bg->cancel(bg); - if (bg->tag) xfree(bg->tag); - xfree(bg); + for (a = a_dead; a; a = aa) { + aa = a->next; + assert(a->f & AF_DEAD); + + /* --- Report what we're doing --- */ + + T( trace(T_ADMIN, "admin: completing destruction of connection %u", + a->seq); ) + + /* --- Abort any background jobs in progress --- */ + + for (bg = a->bg; bg; bg = bbg) { + bbg = bg->next; + bg->cancel(bg); + if (bg->tag) xfree(bg->tag); + xfree(bg); + } + + /* --- Close file descriptors and selectory --- */ + + selbuf_destroy(&a->b); + if (a->b.reader.fd != a->w.fd) close(a->b.reader.fd); + close(a->w.fd); + if (a_stdin == a) a_stdin = 0; + + /* --- Done --- */ + + DESTROY(a); } - if (a->b.reader.fd != a->w.fd) close(a->b.reader.fd); - close(a->w.fd); - - if (a_stdin == a) - a_stdin = 0; - if (a->next) - a->next->prev = a->prev; - if (a->prev) - a->prev->next = a->next; - else - admins = a->next; - DESTROY(a); -} -/* --- @a_unlock@ --- * - * - * Arguments: @admin *a@ = pointer to an admin block - * - * Returns: --- - * - * Use: Unlocks an admin block, allowing its destruction. This is - * also the second half of @a_destroy@. - */ + /* --- All pending destruction completed --- */ -static void a_unlock(admin *a) -{ - assert(a->ref); - if (!--a->ref && (a->f & AF_DEAD)) - a_dodestroy(a); + a_dead = 0; } /* --- @a_destroy@ --- * @@ -1543,28 +1524,21 @@ static void freequeue(oqueue *q) static void a_destroy(admin *a) { - /* --- Don't multiply destroy admin blocks --- */ - if (a->f & AF_DEAD) return; - /* --- Make sure nobody expects it to work --- */ - - a->f |= AF_DEAD; - T( trace(T_ADMIN, "admin: destroying connection %u", a->seq); ) - - /* --- Free the output buffers --- */ + if (a->next) a->next->prev = a->prev; + if (a->prev) a->prev->next = a->next; + else admins = a->next; - if (a->out.hd) - sel_rmfile(&a->w); + if (a->out.hd) sel_rmfile(&a->w); freequeue(&a->out); - /* --- If the block is locked, that's all we can manage --- */ + a->f |= AF_DEAD; + a->next = a_dead; + a_dead = a; - if (!a->ref) - a_dodestroy(a); - T( else - trace(T_ADMIN, "admin: deferring destruction..."); ) + T( trace(T_ADMIN, "admin: killing connection %u", a->seq); ) } /* --- @a_line@ --- * @@ -1608,11 +1582,8 @@ static void a_line(char *p, size_t len, void *vp) a_fail(a, "bad-syntax", "%s", c->name, "", A_END); else a_fail(a, "bad-syntax", "%s", c->name, "%s", c->help, A_END); - } else { - a_lock(a); + } else c->func(a, ac, av + 1); - a_unlock(a); - } return; } } @@ -1678,6 +1649,19 @@ static void a_accept(int fd, unsigned mode, void *v) a_create(nfd, nfd, 0); } +/* --- @a_preselect@ --- * + * + * Arguments: --- + * + * Returns: --- + * + * Use: Informs the admin module that we're about to select again, + * and that it should do cleanup things it has delayed until a + * `safe' time. + */ + +void a_preselect(void) { if (a_dead) a_destroypending(); } + /* --- @a_daemon@ --- * * * Arguments: --- diff --git a/server/tripe.c b/server/tripe.c index be37617a..dd385fbd 100644 --- a/server/tripe.c +++ b/server/tripe.c @@ -341,6 +341,7 @@ int main(int argc, char *argv[]) sel_addtimer(&sel, &it, &tv, interval, 0); for (;;) { + a_preselect(); if (!sel_select(&sel)) selerr = 0; else if (errno != EINTR && errno != EAGAIN) { diff --git a/server/tripe.h b/server/tripe.h index 6bee0c3d..a9a785dc 100644 --- a/server/tripe.h +++ b/server/tripe.h @@ -792,6 +792,19 @@ extern void a_create(int /*fd_in*/, int /*fd_out*/, unsigned /*f*/); extern void a_quit(void); +/* --- @a_preselect@ --- * + * + * Arguments: --- + * + * Returns: --- + * + * Use: Informs the admin module that we're about to select again, + * and that it should do cleanup things it has delayed until a + * `safe' time. + */ + +extern void a_preselect(void); + /* --- @a_daemon@ --- * * * Arguments: --- -- [mdw]