From e3729dd859653f378471eba07ffe01996b8f4fca Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 27 Apr 2010 16:57:52 +0100 Subject: [PATCH] rationalise use of close, postfork, etc.; proper error handling on most closes --- backends/innduct.c | 176 +++++++++++++++++++++++---------------------- 1 file changed, 89 insertions(+), 87 deletions(-) diff --git a/backends/innduct.c b/backends/innduct.c index 385c641..858fa0e 100644 --- a/backends/innduct.c +++ b/backends/innduct.c @@ -70,8 +70,8 @@ OVERALL STATES: - START - | + START + | ,-->--. check F, D | | | | | | @@ -133,14 +133,14 @@ | V duct unlinks D D: duct reading | | | `--<--' | duct finishes - | processing D - | duct unlinks D - | duct exits - V - Dropped - F: ENOENT - D: ENOENT - duct not running + | processing D + | duct unlinks D + | duct exits + V + Dropped + F: ENOENT + D: ENOENT + duct not running "duct reading" means innduct is reading the file but also overwriting processed tokens. @@ -287,8 +287,7 @@ static void queue_check_input_done(void); static void statemc_check_flushing_done(void); static void statemc_check_backlog_done(void); -static void postfork(const char *what); -static void postfork_inputfile(InputFile *ipf); +static void postfork(void); static void open_defer(void); static void close_defer(void); @@ -566,8 +565,8 @@ static pid_t xfork(const char *what) { child= fork(); if (child==-1) sysdie("cannot fork for %s",what); - if (!child) postfork(what); debug("forked %s %ld", what, (unsigned long)child); + if (!child) postfork(); return child; } @@ -690,14 +689,19 @@ static int isewouldblock(int errnoval) { /*========== management of connections ==========*/ +static void conn_closefd(Conn *conn, const char *msgprefix) { + int r= close_perhaps(&conn->fd); + if (r) info("C%d %serror closing socket: %s", + conn->fd, msgprefix, strerror(errno)); +} + static void conn_dispose(Conn *conn) { if (!conn) return; if (conn->fd) { loop->cancel_fd(loop, conn->fd, OOP_WRITE); loop->cancel_fd(loop, conn->fd, OOP_EXCEPTION); } - int r= close_perhaps(&conn->fd); - if (r) info("C%d error closing socket: %s", conn->fd, strerror(errno)); + conn_closefd(conn,""); free(conn); until_connect= reconnect_delay_periods; } @@ -797,16 +801,10 @@ static void check_idle_conns(void) { /*---------- making new connections ----------*/ -static int connecting_sockets[2]= {-1,-1}; static pid_t connecting_child; +static int connecting_fdpass_sock; static void connect_attempt_discard(void) { - if (connecting_sockets[0]) - cancel_fd_read_except(connecting_sockets[0]); - - xclose_perhaps(&connecting_sockets[0], "connecting socketpair (read)",0); - xclose_perhaps(&connecting_sockets[1], "connecting socketpair (write)",0); - if (connecting_child) { int r= kill(connecting_child, SIGTERM); if (r) syswarn("failed to kill connecting child"); @@ -816,6 +814,10 @@ static void connect_attempt_discard(void) { (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL))) report_child_status("connect", status); } + if (connecting_fdpass_sock) { + cancel_fd_read_except(connecting_fdpass_sock); + xclose_perhaps(&connecting_fdpass_sock, "connecting fdpass socket",0); + } } #define PREP_DECL_MSG_CMSG(msg) \ @@ -828,6 +830,8 @@ static void connect_attempt_discard(void) { static void *connchild_event(oop_source *lp, int fd, oop_event e, void *u) { Conn *conn= 0; + assert(fd == connecting_fdpass_sock); + conn= xmalloc(sizeof(*conn)); memset(conn,0,sizeof(*conn)); @@ -857,11 +861,11 @@ static void *connchild_event(oop_source *lp, int fd, oop_event e, void *u) { } else { /* child is still running apparently, report the socket problem */ if (rs < 0) - syswarn("connect: read from child socket failed"); + syswarn("connect: read from fdpass socket failed"); else if (e == OOP_EXCEPTION) - warn("connect: unexpected exception on child socket"); + warn("connect: unexpected exception on fdpass socket"); else if (!rs) - warn("connect: unexpected EOF on child socket"); + warn("connect: unexpected EOF on fdpass socket"); else fatal("connect: unexpected lack of cmsg from child"); } @@ -923,14 +927,14 @@ static int allow_connect_start(void) { } static void connect_start(void) { - assert(!connecting_sockets[0]); - assert(!connecting_sockets[1]); assert(!connecting_child); + assert(!connecting_fdpass_sock); notice("starting connection attempt"); - int r= socketpair(AF_UNIX, SOCK_STREAM, 0, connecting_sockets); - if (r) { syswarn("connect: cannot create socketpair for child"); goto x; } + int socks[2]; + int r= socketpair(AF_UNIX, SOCK_STREAM, 0, socks); + if (r) { syswarn("connect: cannot create socketpair for child"); return; } connecting_child= xfork("connection"); @@ -939,8 +943,7 @@ static void connect_start(void) { char buf[NNTP_STRLEN+100]; int exitstatus= CONNCHILD_ESTATUS_NOSTREAM; - r= close(connecting_sockets[0]); - if (r) sysdie("connect: close parent socket in child"); + xclose(socks[0], "(in child) parent's connection fdpass socket",0); alarm(connection_setup_timeout); if (NNTPconnect((char*)remote_host, port, &cn_from, &cn_to, buf) < 0) { @@ -996,20 +999,15 @@ static void connect_start(void) { memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd)); msg.msg_controllen= cmsg->cmsg_len; - r= sendmsg(connecting_sockets[1], &msg, 0); + r= sendmsg(socks[1], &msg, 0); if (r) sysdie("sendmsg failed for new connection"); _exit(exitstatus); } - r= close(connecting_sockets[1]); connecting_sockets[1]= 0; - if (r) sysdie("connect: close child socket in parent"); - - on_fd_read_except(connecting_sockets[0], connchild_event); - return; - - x: - connect_attempt_discard(); + xclose(socks[1], "connecting fdpass child's socket",0); + connecting_fdpass_sock= socks[0]; + on_fd_read_except(connecting_fdpass_sock, connchild_event); } /*---------- assigning articles to conns, and transmitting ----------*/ @@ -1018,7 +1016,7 @@ static void check_assign_articles(void) { for (;;) { if (!queue.count) break; - + Conn *walk, *use=0; int spare, inqueue; @@ -1087,7 +1085,7 @@ static void conn_maybe_write(Conn *conn) { /*========== article transmission ==========*/ static XmitDetails *xmit_core(Conn *conn, const char *data, int len, - XmitKind kind) { /* caller must then fill in details */ + XmitKind kind) { /* caller must then fill in details */ struct iovec *v= &conn->xmit[conn->xmitu]; XmitDetails *d= &conn->xmitd[conn->xmitu++]; v->iov_base= (char*)data; @@ -1227,7 +1225,7 @@ static void *peer_rd_err(oop_source *lp, oop_read *oread, oop_event ev, static Article *article_reply_check(Conn *conn, const char *response, int code_indicates_streaming, int must_have_sent - /* 1:yes, -1:no, 0:dontcare */, + /* 1:yes, -1:no, 0:dontcare */, const char *sanitised_response) { Article *art= LIST_HEAD(conn->sent); @@ -1529,7 +1527,7 @@ static void *feedfile_got_article(oop_source *lp, oop_read *rd, ipf->readcount_blank++; return OOP_CONTINUE; } - + char *space= strchr(data,' '); int tokenlen= space-data; int midlen= (int)recsz-tokenlen-1; @@ -1780,16 +1778,16 @@ static void inputfile_reading_stop(InputFile *ipf) { .=======. ||START|| - `=======' - | - | open F - | - | F ENOENT - |`---------------------------------------------------. + `=======' + | + | open F + | + | F ENOENT + |`---------------------------------------------------. F OPEN OK | | - |`---------------- - - - | + |`---------------- - - - | D ENOENT | D EXISTS see OVERALL STATES diagram | - | for full startup logic | + | for full startup logic | ,--------->| | | V | | ============ try to | @@ -1831,7 +1829,7 @@ static void inputfile_reading_stop(InputFile *ipf) { | V | | V | ============= V V ============ | SEPARATED-1 | | DROPPING-1 - | flsh->fd>0 | | flsh->fd>0 + | flsh->rd!=0 | | flsh->rd!=0 | [Separated] | | [Dropping] | main F idle | | main none | old D tail | | old D tail @@ -1841,7 +1839,7 @@ static void inputfile_reading_stop(InputFile *ipf) { | V | | V | =============== | | =============== | SEPARATED-2 | | DROPPING-2 - | flsh->fd==0 | V flsh->fd==0 + | flsh->rd==0 | V flsh->rd==0 | [Finishing] | | [Dropping] | main F tail | `. main none | old D closed | `. old D closed @@ -1854,22 +1852,22 @@ static void inputfile_reading_stop(InputFile *ipf) { | | | | | | V V `----------' ============== - DROPPED - [Dropped] - main none - old none - some backlog - ============== - | - | ALL BACKLOG DONE - | - | unlink lock - | exit - V - ========== - (ESRCH) - [Droppped] - ========== + DROPPED + [Dropped] + main none + old none + some backlog + ============== + | + | ALL BACKLOG DONE + | + | unlink lock + | exit + V + ========== + (ESRCH) + [Droppped] + ========== * ->8- */ @@ -1911,8 +1909,7 @@ static void statemc_init(void) { if (!lock_noent && samefile(&stab, &stabf)) break; - if (close(lockfd)) - sysdie("could not close stale lockfile %s", path_lock); + xclose(lockfd, "stale lockfile ", path_lock); } debug("startup: locked"); @@ -2051,7 +2048,7 @@ static void statemc_check_backlog_done(void) { const char *rest= under ? under+1 : leaf; if (!strncmp(rest,"backlog",7)) rest += 7; notice_processed(ipf,"backlog:",rest); - + close_input_file(ipf); if (unlink(ipf->path)) { if (errno != ENOENT) @@ -2313,12 +2310,14 @@ static void search_backlog_file(void) { /*========== flushing the feed ==========*/ static pid_t inndcomm_child; +static int inndcomm_sentinel_fd; static void *inndcomm_event(oop_source *lp, int fd, oop_event e, void *u) { assert(inndcomm_child); + assert(fd == inndcomm_sentinel_fd); int status= xwaitpid(&inndcomm_child, "inndcomm"); cancel_fd_read_except(fd); - close(fd); + xclose_perhaps(&fd, "inndcomm sentinel pipe",0); assert(!flushing_input_file); @@ -2332,7 +2331,7 @@ static void *inndcomm_event(oop_source *lp, int fd, oop_event e, void *u) { warn("feed has been dropped by innd, finishing up"); flushing_input_file= main_input_file; tailing_queue_readable(flushing_input_file); - /* we probably previously returned EAGAIN from our fake read method + /* we probably previously returned EAGAIN from our fake read method * when in fact we were at EOF, so signal another readable event * so we actually see the EOF */ @@ -2392,6 +2391,7 @@ void spawn_inndcomm_flush(const char *why) { /* Moved => Flushing */ assert(sms==sm_NORMAL || sms==sm_FLUSHFAILED); assert(!inndcomm_child); + assert(!inndcomm_sentinel_fd); if (pipe(pipefds)) sysdie("create pipe for inndcomm child sentinel"); @@ -2402,7 +2402,8 @@ void spawn_inndcomm_flush(const char *why) { /* Moved => Flushing */ char *reply; int r; - close(pipefds[0]); + xclose(pipefds[0], "(in child) inndcomm sentinel parent's end",0); + /* parent spots the autoclose of pipefds[1] when we die or exit */ alarm(inndcomm_flush_timeout); r= ICCopen(); if (r) inndcommfail("connect"); @@ -2414,9 +2415,10 @@ void spawn_inndcomm_flush(const char *why) { /* Moved => Flushing */ exit(INNDCOMMCHILD_ESTATUS_FAIL); } - close(pipefds[1]); - int sentinel_fd= pipefds[0]; - on_fd_read_except(sentinel_fd, inndcomm_event); + xclose(pipefds[1], "inndcomm sentinel child's end",0); + inndcomm_sentinel_fd= pipefds[0]; + assert(inndcomm_sentinel_fd); + on_fd_read_except(inndcomm_sentinel_fd, inndcomm_event); SMS(FLUSHING, 0, why); } @@ -2431,19 +2433,19 @@ static void postfork_inputfile(InputFile *ipf) { static void postfork_stdio(FILE *f, const char *what, const char *what2) { /* we have no stdio streams that are buffered long-term */ if (!f) return; - if (fclose(f)) sysdie("close (in child) %s%s", what, what2?what2:0); + if (fclose(f)) sysdie("(in child) close %s%s", what, what2?what2:0); } -static void postfork(const char *what) { +static void postfork(void) { if (signal(SIGPIPE, SIG_DFL) == SIG_ERR) - sysdie("%s child: failed to reset SIGPIPE"); + sysdie("(in child) failed to reset SIGPIPE"); postfork_inputfile(main_input_file); postfork_inputfile(flushing_input_file); Conn *conn; for (conn=LIST_HEAD(conns); conn; conn=LIST_NEXT(conn)) - close_perhaps(&conn->fd); + conn_closefd(conn,"(in child) "); postfork_stdio(defer, "defer file ", path_defer); } @@ -2700,6 +2702,9 @@ int main(int argc, char **argv) { if (signal(SIGPIPE, SIG_IGN) == SIG_ERR) sysdie("could not ignore SIGPIPE"); + LIST_INIT(conns); + LIST_INIT(queue); + if (become_daemon) { int i; for (i=3; i<255; i++) @@ -2712,7 +2717,7 @@ int main(int argc, char **argv) { dup2(null,0); dup2(null,1); dup2(null,2); - close(null); + xclose(null, "/dev/null original fd",0); pid_t child1= xfork("daemonise first fork"); if (child1) _exit(0); @@ -2726,9 +2731,6 @@ int main(int argc, char **argv) { notice("starting"); - LIST_INIT(conns); - LIST_INIT(queue); - if (!filemon_method_init()) { warn("no file monitoring available, polling"); filepoll_schedule(); -- 2.30.2