Remove guestfs___print_timestamped_argv.

This function was used to print the qemu and
febootstrap-supermin-helper command lines.

Unfortunately in the qemu case it was used incorrectly: it called the
internal debug function (ie. event API callback) from the forked qemu
subprocess, which meant that higher level event callbacks might have
been invoked from the child process.

To fix this, convert the qemu case into a new function called
print_qemu_command line which just prints the command line directly to
stderr.  This is called after stderr has been redirected into the pipe
to the main process.  Thus the qemu command line will be marshalled
into the event API along with other qemu and appliance output.

After fixing this, only one use of guestfs___print_timestamped_argv
remained, for printing the febootstrap-supermin-helper command line.
This is converted to a local function print_febootstrap_command_line.

Also print_febootstrap_command_line is now called before we fork
febootstrap-supermin-helper, so that messages no longer overlap.

diff --git a/src/appliance.c b/src/appliance.c
index 5683882..1df8c36 100644 (file)
--- a/src/appliance.c
+++ b/src/appliance.c
@@ -55,6 +55,7 @@ static int check_for_cached_appliance (guestfs_h *g, const char *supermin_path,
 static int build_supermin_appliance (guestfs_h *g, const char *supermin_path, const char *checksum, uid_t uid, char **kernel, char **initrd, char **appliance);
 static int hard_link_to_cached_appliance (guestfs_h *g, const char *cachedir, char **kernel, char **initrd, char **appliance);
 static int run_supermin_helper (guestfs_h *g, const char *supermin_path, const char *cachedir, size_t cdlen);
+static void print_febootstrap_command_line (guestfs_h *g, const char *argv[]);
 
 /* Locate or build the appliance.
  *
@@ -640,6 +641,9 @@ run_supermin_helper (guestfs_h *g, const char *supermin_path,
   argv[i++] = root;
   argv[i++] = NULL;
 
+  if (g->verbose)
+    print_febootstrap_command_line (g, argv);
+
   pid_t pid = fork ();
   if (pid == -1) {
     perrorf (g, "fork");
@@ -647,9 +651,6 @@ run_supermin_helper (guestfs_h *g, const char *supermin_path,
   }
 
   if (pid > 0) {                /* Parent. */
-    if (g->verbose)
-      guestfs___print_timestamped_argv (g, argv);
-
     int status;
     if (waitpid (pid, &status, 0) == -1) {
       perrorf (g, "waitpid");
@@ -674,6 +675,54 @@ run_supermin_helper (guestfs_h *g, const char *supermin_path,
   _exit (EXIT_FAILURE);
 }
 
+static void
+print_febootstrap_command_line (guestfs_h *g, const char *argv[])
+{
+  int i;
+  int needs_quote;
+  char *buf;
+  size_t len;
+
+  /* Calculate length of the buffer needed.  This is an overestimate. */
+  len = 0;
+  for (i = 0; argv[i] != NULL; ++i)
+    len += strlen (argv[i]) + 32;
+
+  buf = malloc (len);
+  if (buf == NULL) {
+    warning (g, "malloc: %m");
+    return;
+  }
+
+  len = 0;
+  for (i = 0; argv[i] != NULL; ++i) {
+    if (i > 0) {
+      strcpy (&buf[len], " ");
+      len++;
+    }
+
+    /* Does it need shell quoting?  This only deals with simple cases. */
+    needs_quote = strcspn (argv[i], " ") != strlen (argv[i]);
+
+    if (needs_quote) {
+      strcpy (&buf[len], "'");
+      len++;
+    }
+
+    strcpy (&buf[len], argv[i]);
+    len += strlen (argv[i]);
+
+    if (needs_quote) {
+      strcpy (&buf[len], "'");
+      len++;
+    }
+  }
+
+  guestfs___print_timestamped_message (g, "%s", buf);
+
+  free (buf);
+}
+
 /* Search elements of g->path, returning the first path element which
  * matches the predicate function 'pred'.
  *

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index d2deb1c..79d8990 100644 (file)
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -328,7 +328,6 @@ extern void guestfs___debug (guestfs_h *g, const char *fs, ...)
 extern void guestfs___trace (guestfs_h *g, const char *fs, ...)
   __attribute__((format (printf,2,3)));
 extern const char *guestfs___persistent_tmpdir (void);
-extern void guestfs___print_timestamped_argv (guestfs_h *g, const char *argv[]);
 extern void guestfs___print_timestamped_message (guestfs_h *g, const char *fs, ...);
 extern void guestfs___free_inspect_info (guestfs_h *g);
 extern int guestfs___set_busy (guestfs_h *g);

diff --git a/src/launch.c b/src/launch.c
index e58add5..234f42e 100644 (file)
--- a/src/launch.c
+++ b/src/launch.c
@@ -73,6 +73,7 @@
 
 static int launch_appliance (guestfs_h *g);
 static int64_t timeval_diff (const struct timeval *x, const struct timeval *y);
+static void print_qemu_command_line (guestfs_h *g, char **argv);
 static int connect_unix_socket (guestfs_h *g, const char *sock);
 static int qemu_supports (guestfs_h *g, const char *option);
 
@@ -650,9 +651,6 @@ launch_appliance (guestfs_h *g)
     incr_cmdline_size (g);
     g->cmdline[g->cmdline_size-1] = NULL;
 
-    if (g->verbose)
-      guestfs___print_timestamped_argv (g, (const char **)g->cmdline);
-
     if (!g->direct) {
       /* Set up stdin, stdout, stderr. */
       close (0);
@@ -682,6 +680,10 @@ launch_appliance (guestfs_h *g)
       close (rfd[1]);
     }
 
+    /* Dump the command line (after setting up stderr above). */
+    if (g->verbose)
+      print_qemu_command_line (g, g->cmdline);
+
     /* Put qemu in a new process group. */
     if (g->pgroup)
       setpgid (0, 0);
@@ -1002,47 +1004,9 @@ timeval_diff (const struct timeval *x, const struct timeval *y)
   return msec;
 }
 
-void
-guestfs___print_timestamped_argv (guestfs_h *g, const char * argv[])
-{
-  int i = 0;
-  int needs_quote;
-  char *buf = NULL;
-  size_t len;
-  FILE *fp;
-
-  fp = open_memstream (&buf, &len);
-  if (fp == NULL) {
-    warning (g, "open_memstream: %m");
-    return;
-  }
-
-  struct timeval tv;
-  gettimeofday (&tv, NULL);
-  fprintf (fp, "[%05" PRIi64 "ms] ", timeval_diff (&g->launch_t, &tv));
-
-  while (argv[i]) {
-    if (argv[i][0] == '-') /* -option starts a new line */
-      fprintf (fp, " \\\n   ");
-
-    if (i > 0) fputc (' ', fp);
-
-    /* Does it need shell quoting?  This only deals with simple cases. */
-    needs_quote = strcspn (argv[i], " ") != strlen (argv[i]);
-
-    if (needs_quote) fputc ('\'', fp);
-    fprintf (fp, "%s", argv[i]);
-    if (needs_quote) fputc ('\'', fp);
-    i++;
-  }
-
-  fclose (fp);
-
-  debug (g, "%s", buf);
-
-  free (buf);
-}
-
+/* Note that since this calls 'debug' it should only be called
+ * from the parent process.
+ */
 void
 guestfs___print_timestamped_message (guestfs_h *g, const char *fs, ...)
 {
@@ -1064,6 +1028,36 @@ guestfs___print_timestamped_message (guestfs_h *g, const char *fs, ...)
   free (msg);
 }
 
+/* This is called from the forked subprocess just before qemu runs, so
+ * it can just print the message straight to stderr, where it will be
+ * picked up and funnelled through the usual appliance event API.
+ */
+static void
+print_qemu_command_line (guestfs_h *g, char **argv)
+{
+  int i = 0;
+  int needs_quote;
+
+  struct timeval tv;
+  gettimeofday (&tv, NULL);
+  fprintf (stderr, "[%05" PRIi64 "ms] ", timeval_diff (&g->launch_t, &tv));
+
+  while (argv[i]) {
+    if (argv[i][0] == '-') /* -option starts a new line */
+      fprintf (stderr, " \\\n   ");
+
+    if (i > 0) fputc (' ', stderr);
+
+    /* Does it need shell quoting?  This only deals with simple cases. */
+    needs_quote = strcspn (argv[i], " ") != strlen (argv[i]);
+
+    if (needs_quote) fputc ('\'', stderr);
+    fprintf (stderr, "%s", argv[i]);
+    if (needs_quote) fputc ('\'', stderr);
+    i++;
+  }
+}
+
 static int read_all (guestfs_h *g, FILE *fp, char **ret);
 
 /* Test qemu binary (or wrapper) runs, and do 'qemu -help' and