chiark / gitweb /
rationalise use of close, postfork, etc.; proper error handling on most closes
authorIan Jackson <ian@liberator.(none)>
Tue, 27 Apr 2010 15:57:52 +0000 (16:57 +0100)
committerIan Jackson <ian@liberator.(none)>
Tue, 27 Apr 2010 15:57:52 +0000 (16:57 +0100)
backends/innduct.c

index 385c641..858fa0e 100644 (file)
@@ -70,8 +70,8 @@
 
    OVERALL STATES:
 
-                                                               START
-                                                                  |
+                                                               START
+                                                                 |
      ,-->--.                                                 check F, D
      |     |                                                      |
      |     |                                                      |
      |     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();