Experimental recovery process should help with not cleaning up qemu.

[libguestfs.git] / src / guestfs.c

diff --git a/src/guestfs.c b/src/guestfs.c
index fdf5fed..05fb113 100644 (file)
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -143,7 +143,8 @@ struct guestfs_h
 
   int fd[2];                   /* Stdin/stdout of qemu. */
   int sock;                    /* Daemon communications socket. */
-  int pid;                     /* Qemu PID. */
+  pid_t pid;                   /* Qemu PID. */
+  pid_t recoverypid;           /* Recovery process PID. */
   time_t start_t;              /* The time when we started qemu. */
 
   int stdout_watch;            /* Watches qemu stdout for log messages. */
@@ -842,6 +843,40 @@ guestfs_launch (guestfs_h *g)
   /* Parent (library). */
   g->pid = r;
 
+  /* Fork the recovery process off which will kill qemu if the parent
+   * process fails to do so (eg. if the parent segfaults).
+   */
+  r = fork ();
+  if (r == 0) {
+    pid_t qemu_pid = g->pid;
+    pid_t parent_pid = getppid ();
+
+    /* Writing to argv is hideously complicated and error prone.  See:
+     * http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/ps_status.c?rev=1.33.2.1;content-type=text%2Fplain
+     */
+
+    /* Loop around waiting for one or both of the other processes to
+     * disappear.  It's fair to say this is very hairy.  The PIDs that
+     * we are looking at might be reused by another process.  We are
+     * effectively polling.  Is the cure worse than the disease?
+     */
+    for (;;) {
+      if (kill (qemu_pid, 0) == -1) /* qemu's gone away, we aren't needed */
+       _exit (0);
+      if (kill (parent_pid, 0) == -1) {
+       /* Parent's gone away, qemu still around, so kill qemu. */
+       kill (qemu_pid, 9);
+       _exit (0);
+      }
+      sleep (2);
+    }
+  }
+
+  /* Don't worry, if the fork failed, this will be -1.  The recovery
+   * process isn't essential.
+   */
+  g->recoverypid = r;
+
   /* Start the clock ... */
   time (&g->start_t);
 
@@ -938,11 +973,14 @@ guestfs_launch (guestfs_h *g)
   close (wfd[1]);
   close (rfd[0]);
   kill (g->pid, 9);
+  if (g->recoverypid > 0) kill (g->recoverypid, 9);
   waitpid (g->pid, NULL, 0);
+  if (g->recoverypid > 0) waitpid (g->recoverypid, NULL, 0);
   g->fd[0] = -1;
   g->fd[1] = -1;
   g->sock = -1;
   g->pid = 0;
+  g->recoverypid = 0;
   g->start_t = 0;
   g->stdout_watch = -1;
   g->sock_watch = -1;
@@ -1018,6 +1056,7 @@ guestfs_kill_subprocess (guestfs_h *g)
     fprintf (stderr, "sending SIGTERM to process %d\n", g->pid);
 
   kill (g->pid, SIGTERM);
+  if (g->recoverypid > 0) kill (g->recoverypid, 9);
 
   return 0;
 }
@@ -1135,7 +1174,9 @@ stdout_event (struct guestfs_main_loop *ml, guestfs_h *g, void *data,
     if (g->verbose)
       fprintf (stderr, "stdout_event: %p: child process died\n", g);
     /*kill (g->pid, SIGTERM);*/
+    if (g->recoverypid > 0) kill (g->recoverypid, 9);
     waitpid (g->pid, NULL, 0);
+    if (g->recoverypid > 0) waitpid (g->recoverypid, NULL, 0);
     if (g->stdout_watch >= 0)
       g->main_loop->remove_handle (g->main_loop, g, g->stdout_watch);
     if (g->sock_watch >= 0)
@@ -1147,6 +1188,7 @@ stdout_event (struct guestfs_main_loop *ml, guestfs_h *g, void *data,
     g->fd[1] = -1;
     g->sock = -1;
     g->pid = 0;
+    g->recoverypid = 0;
     g->start_t = 0;
     g->stdout_watch = -1;
     g->sock_watch = -1;
@@ -1667,10 +1709,9 @@ send_file_complete_sync (guestfs_h *g)
 static int check_for_daemon_cancellation (guestfs_h *g);
 
 static int
-send_file_chunk_sync (guestfs_h *g, int cancel, const char *buf, size_t len)
+send_file_chunk_sync (guestfs_h *g, int cancel, const char *buf, size_t buflen)
 {
-  char data[GUESTFS_MAX_CHUNK_SIZE + 48];
-  unsigned datalen;
+  unsigned len;
   int sent;
   guestfs_chunk chunk;
   XDR xdr;
@@ -1681,6 +1722,14 @@ send_file_chunk_sync (guestfs_h *g, int cancel, const char *buf, size_t len)
     return -1;
   }
 
+  /* This is probably an internal error.  Or perhaps we should just
+   * free the buffer anyway?
+   */
+  if (g->msg_out != NULL) {
+    error (g, "guestfs__send_sync: msg_out should be NULL");
+    return -1;
+  }
+
   /* Did the daemon send a cancellation message? */
   if (check_for_daemon_cancellation (g)) {
     if (g->verbose)
@@ -1688,35 +1737,34 @@ send_file_chunk_sync (guestfs_h *g, int cancel, const char *buf, size_t len)
     return -2;
   }
 
+  /* Allocate the chunk buffer.  Don't use the stack to avoid
+   * excessive stack usage and unnecessary copies.
+   */
+  g->msg_out = safe_malloc (g, GUESTFS_MAX_CHUNK_SIZE + 4 + 48);
+  xdrmem_create (&xdr, g->msg_out + 4, GUESTFS_MAX_CHUNK_SIZE + 48, XDR_ENCODE);
+
   /* Serialize the chunk. */
   chunk.cancel = cancel;
-  chunk.data.data_len = len;
+  chunk.data.data_len = buflen;
   chunk.data.data_val = (char *) buf;
 
-  if (g->verbose)
-    fprintf (stderr,
-            "library sending chunk cancel = %d, len = %zu, buf = %p\n",
-            cancel, len, buf);
-
-  xdrmem_create (&xdr, data, sizeof data, XDR_ENCODE);
   if (!xdr_guestfs_chunk (&xdr, &chunk)) {
-    error (g, "xdr_guestfs_chunk failed (buf = %p, len = %zu)", buf, len);
+    error (g, "xdr_guestfs_chunk failed (buf = %p, buflen = %zu)",
+          buf, buflen);
     xdr_destroy (&xdr);
-    return -1;
+    goto cleanup1;
   }
 
-  datalen = xdr_getpos (&xdr);
+  len = xdr_getpos (&xdr);
   xdr_destroy (&xdr);
 
-  /* Allocate outgoing message buffer. */
-  g->msg_out = safe_malloc (g, datalen + 4);
-  g->msg_out_size = datalen + 4;
+  /* Reduce the size of the outgoing message buffer to the real length. */
+  g->msg_out = safe_realloc (g, g->msg_out, len + 4);
+  g->msg_out_size = len + 4;
   g->msg_out_pos = 0;
 
   xdrmem_create (&xdr, g->msg_out, 4, XDR_ENCODE);
-  xdr_uint32_t (&xdr, &datalen);
-
-  memcpy (g->msg_out + 4, data, datalen);
+  xdr_uint32_t (&xdr, &len);
 
   if (guestfs__switch_to_sending (g) == -1)
     goto cleanup1;