rpki-client collect childs on pipe hangup

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

rpki-client collect childs on pipe hangup

Claudio Jeker
Currently when a pipe to some child is closed the main process errors out
hard. This is not great since the exit reason is not shown.
Change this to break out of the poll loop and also restructure the wait
code to use a loop which checks for both exit and signal status.
I also switched rsync and proc in the pfds and replaced an abort with an
errx call.

With this I see which signal killed a process e.g. if I kill -9 rsync:
...
rpki-client: ta/afrinic: loaded from network
rpki-client: ta/apnic: loaded from network
rpki-client: ta/lacnic: loaded from network
rpki-client: poll[1]: hangup
rpki-client: rsync terminated signal 9

--
:wq Claudio

Index: main.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.131
diff -u -p -r1.131 main.c
--- main.c 7 Apr 2021 16:06:37 -0000 1.131
+++ main.c 8 Apr 2021 08:48:46 -0000
@@ -538,7 +538,7 @@ entity_process(int proc, struct stats *s
  st->gbrs++;
  break;
  default:
- abort();
+ errx(1, "unknown entity type");
  }
 
  entity_queue--;
@@ -580,7 +580,6 @@ void
 suicide(int sig __attribute__((unused)))
 {
  killme = 1;
-
 }
 
 #define NPFD 4
@@ -589,9 +588,9 @@ int
 main(int argc, char *argv[])
 {
  int rc = 1, c, st, proc, rsync, http, rrdp, ok,
- fl = SOCK_STREAM | SOCK_CLOEXEC;
+ hangup = 0, fl = SOCK_STREAM | SOCK_CLOEXEC;
  size_t i, id, outsz = 0, talsz = 0;
- pid_t procpid, rsyncpid, httppid, rrdppid;
+ pid_t pid, procpid, rsyncpid, httppid, rrdppid;
  int fd[2];
  struct pollfd pfd[NPFD];
  struct msgbuf *queues[NPFD];
@@ -599,7 +598,7 @@ main(int argc, char *argv[])
  char *rsync_prog = "openrsync";
  char *bind_addr = NULL;
  const char *cachedir = NULL, *outputdir = NULL;
- const char *tals[TALSZ_MAX], *errs;
+ const char *tals[TALSZ_MAX], *errs, *name;
  struct vrp_tree v = RB_INITIALIZER(&v);
  struct rusage ru;
  struct timeval start_time, now_time;
@@ -871,10 +870,10 @@ main(int argc, char *argv[])
  * parsing process.
  */
 
- pfd[0].fd = rsync;
- queues[0] = &rsyncq;
- pfd[1].fd = proc;
- queues[1] = &procq;
+ pfd[0].fd = proc;
+ queues[0] = &procq;
+ pfd[1].fd = rsync;
+ queues[1] = &rsyncq;
  pfd[2].fd = http;
  queues[2] = &httpq;
  pfd[3].fd = rrdp;
@@ -909,8 +908,10 @@ main(int argc, char *argv[])
  for (i = 0; i < NPFD; i++) {
  if (pfd[i].revents & (POLLERR|POLLNVAL))
  errx(1, "poll[%zu]: bad fd", i);
- if (pfd[i].revents & POLLHUP)
- errx(1, "poll[%zu]: hangup", i);
+ if (pfd[i].revents & POLLHUP) {
+ warnx("poll[%zu]: hangup", i);
+ hangup = 1;
+ }
  if (pfd[i].revents & POLLOUT) {
  /*
  * XXX work around deadlocks because of
@@ -929,7 +930,8 @@ main(int argc, char *argv[])
  io_socket_blocking(pfd[i].fd);
  }
  }
-
+ if (hangup)
+ break;
 
  /*
  * Check the rsync and http process.
@@ -938,7 +940,7 @@ main(int argc, char *argv[])
  * the parser process.
  */
 
- if ((pfd[0].revents & POLLIN)) {
+ if ((pfd[1].revents & POLLIN)) {
  io_simple_read(rsync, &id, sizeof(id));
  io_simple_read(rsync, &ok, sizeof(ok));
  rsync_finish(id, ok);
@@ -1013,7 +1015,7 @@ main(int argc, char *argv[])
  * Dequeue these one by one.
  */
 
- if ((pfd[1].revents & POLLIN)) {
+ if ((pfd[0].revents & POLLIN)) {
  entity_process(proc, &stats, &v);
  }
  }
@@ -1024,10 +1026,6 @@ main(int argc, char *argv[])
  errx(1, "excessive runtime (%d seconds), giving up", timeout);
  }
 
- assert(entity_queue == 0);
- logx("all files parsed: generating output");
- rc = 0;
-
  /*
  * For clean-up, close the input for the parser and rsync
  * process.
@@ -1039,36 +1037,41 @@ main(int argc, char *argv[])
  close(http);
  close(rrdp);
 
- if (waitpid(procpid, &st, 0) == -1)
- err(1, "waitpid");
- if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
- warnx("parser process exited abnormally");
- rc = 1;
- }
- if (!noop) {
- if (waitpid(rsyncpid, &st, 0) == -1)
- err(1, "waitpid");
- if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
- warnx("rsync process exited abnormally");
- rc = 1;
+ for (;;) {
+ pid = waitpid(WAIT_ANY, &st, 0);
+ if (pid == -1) {
+ if (errno == EINTR)
+ continue;
+ if (errno != ECHILD)
+ err(1, "wait");
+ break;
  }
 
- if (waitpid(httppid, &st, 0) == -1)
- err(1, "waitpid");
- if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
- warnx("http process exited abnormally");
- rc = 1;
- }
+ if (pid == procpid)
+ name = "parser";
+ else if (pid == rsyncpid)
+ name = "rsync";
+ else if (pid == httppid)
+ name = "http";
+ else if (pid == rrdppid)
+ name = "rrdp";
+ else
+ name = "unknown";
 
- if (rrdpon) {
- if (waitpid(rrdppid, &st, 0) == -1)
- err(1, "waitpid");
- if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
- warnx("rrdp process exited abnormally");
- rc = 1;
- }
+ if (WIFSIGNALED(st)) {
+ warnx("%s terminated signal %d", name, WTERMSIG(st));
+ rc = 1;
+ } else if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
+ warnx("%s process exited abnormally", name);
+ rc = 1;
  }
  }
+
+ /* processing did not finish because of error */
+ if (entity_queue != 0)
+ return 1;
+
+ logx("all files parsed: generating output");
 
  repo_cleanup(&fpt);
 

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client collect childs on pipe hangup

Theo Buehler-3
On Thu, Apr 08, 2021 at 10:56:26AM +0200, Claudio Jeker wrote:

> Currently when a pipe to some child is closed the main process errors out
> hard. This is not great since the exit reason is not shown.
> Change this to break out of the poll loop and also restructure the wait
> code to use a loop which checks for both exit and signal status.
> I also switched rsync and proc in the pfds and replaced an abort with an
> errx call.
>
> With this I see which signal killed a process e.g. if I kill -9 rsync:
> ...
> rpki-client: ta/afrinic: loaded from network
> rpki-client: ta/apnic: loaded from network
> rpki-client: ta/lacnic: loaded from network
> rpki-client: poll[1]: hangup
> rpki-client: rsync terminated signal 9

I think you meant to initialize rc = 0 at the top of main. As it is now,
rpki-client will always exit with 1 at 'return rc;' since you dropped
the only place that set rc to 0.

Other than that, this reads ok.

>
> --
> :wq Claudio
>
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.131
> diff -u -p -r1.131 main.c
> --- main.c 7 Apr 2021 16:06:37 -0000 1.131
> +++ main.c 8 Apr 2021 08:48:46 -0000
> @@ -538,7 +538,7 @@ entity_process(int proc, struct stats *s
>   st->gbrs++;
>   break;
>   default:
> - abort();
> + errx(1, "unknown entity type");
>   }
>  
>   entity_queue--;
> @@ -580,7 +580,6 @@ void
>  suicide(int sig __attribute__((unused)))
>  {
>   killme = 1;
> -
>  }
>  
>  #define NPFD 4
> @@ -589,9 +588,9 @@ int
>  main(int argc, char *argv[])
>  {
>   int rc = 1, c, st, proc, rsync, http, rrdp, ok,
> - fl = SOCK_STREAM | SOCK_CLOEXEC;
> + hangup = 0, fl = SOCK_STREAM | SOCK_CLOEXEC;
>   size_t i, id, outsz = 0, talsz = 0;
> - pid_t procpid, rsyncpid, httppid, rrdppid;
> + pid_t pid, procpid, rsyncpid, httppid, rrdppid;
>   int fd[2];
>   struct pollfd pfd[NPFD];
>   struct msgbuf *queues[NPFD];
> @@ -599,7 +598,7 @@ main(int argc, char *argv[])
>   char *rsync_prog = "openrsync";
>   char *bind_addr = NULL;
>   const char *cachedir = NULL, *outputdir = NULL;
> - const char *tals[TALSZ_MAX], *errs;
> + const char *tals[TALSZ_MAX], *errs, *name;
>   struct vrp_tree v = RB_INITIALIZER(&v);
>   struct rusage ru;
>   struct timeval start_time, now_time;
> @@ -871,10 +870,10 @@ main(int argc, char *argv[])
>   * parsing process.
>   */
>  
> - pfd[0].fd = rsync;
> - queues[0] = &rsyncq;
> - pfd[1].fd = proc;
> - queues[1] = &procq;
> + pfd[0].fd = proc;
> + queues[0] = &procq;
> + pfd[1].fd = rsync;
> + queues[1] = &rsyncq;
>   pfd[2].fd = http;
>   queues[2] = &httpq;
>   pfd[3].fd = rrdp;
> @@ -909,8 +908,10 @@ main(int argc, char *argv[])
>   for (i = 0; i < NPFD; i++) {
>   if (pfd[i].revents & (POLLERR|POLLNVAL))
>   errx(1, "poll[%zu]: bad fd", i);
> - if (pfd[i].revents & POLLHUP)
> - errx(1, "poll[%zu]: hangup", i);
> + if (pfd[i].revents & POLLHUP) {
> + warnx("poll[%zu]: hangup", i);
> + hangup = 1;
> + }
>   if (pfd[i].revents & POLLOUT) {
>   /*
>   * XXX work around deadlocks because of
> @@ -929,7 +930,8 @@ main(int argc, char *argv[])
>   io_socket_blocking(pfd[i].fd);
>   }
>   }
> -
> + if (hangup)
> + break;
>  
>   /*
>   * Check the rsync and http process.
> @@ -938,7 +940,7 @@ main(int argc, char *argv[])
>   * the parser process.
>   */
>  
> - if ((pfd[0].revents & POLLIN)) {
> + if ((pfd[1].revents & POLLIN)) {
>   io_simple_read(rsync, &id, sizeof(id));
>   io_simple_read(rsync, &ok, sizeof(ok));
>   rsync_finish(id, ok);
> @@ -1013,7 +1015,7 @@ main(int argc, char *argv[])
>   * Dequeue these one by one.
>   */
>  
> - if ((pfd[1].revents & POLLIN)) {
> + if ((pfd[0].revents & POLLIN)) {
>   entity_process(proc, &stats, &v);
>   }
>   }
> @@ -1024,10 +1026,6 @@ main(int argc, char *argv[])
>   errx(1, "excessive runtime (%d seconds), giving up", timeout);
>   }
>  
> - assert(entity_queue == 0);
> - logx("all files parsed: generating output");
> - rc = 0;
> -
>   /*
>   * For clean-up, close the input for the parser and rsync
>   * process.
> @@ -1039,36 +1037,41 @@ main(int argc, char *argv[])
>   close(http);
>   close(rrdp);
>  
> - if (waitpid(procpid, &st, 0) == -1)
> - err(1, "waitpid");
> - if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
> - warnx("parser process exited abnormally");
> - rc = 1;
> - }
> - if (!noop) {
> - if (waitpid(rsyncpid, &st, 0) == -1)
> - err(1, "waitpid");
> - if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
> - warnx("rsync process exited abnormally");
> - rc = 1;
> + for (;;) {
> + pid = waitpid(WAIT_ANY, &st, 0);
> + if (pid == -1) {
> + if (errno == EINTR)
> + continue;
> + if (errno != ECHILD)
> + err(1, "wait");
> + break;
>   }
>  
> - if (waitpid(httppid, &st, 0) == -1)
> - err(1, "waitpid");
> - if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
> - warnx("http process exited abnormally");
> - rc = 1;
> - }
> + if (pid == procpid)
> + name = "parser";
> + else if (pid == rsyncpid)
> + name = "rsync";
> + else if (pid == httppid)
> + name = "http";
> + else if (pid == rrdppid)
> + name = "rrdp";
> + else
> + name = "unknown";
>  
> - if (rrdpon) {
> - if (waitpid(rrdppid, &st, 0) == -1)
> - err(1, "waitpid");
> - if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
> - warnx("rrdp process exited abnormally");
> - rc = 1;
> - }
> + if (WIFSIGNALED(st)) {
> + warnx("%s terminated signal %d", name, WTERMSIG(st));
> + rc = 1;
> + } else if (!WIFEXITED(st) || WEXITSTATUS(st) != 0) {
> + warnx("%s process exited abnormally", name);
> + rc = 1;
>   }
>   }
> +
> + /* processing did not finish because of error */
> + if (entity_queue != 0)
> + return 1;
> +
> + logx("all files parsed: generating output");
>  
>   repo_cleanup(&fpt);
>  
>