Libguestfs - Nov 2009 - [PATCH 0/2] Two small fixes to command*() functions in the daemon

The command*() functions are sane wrappers we use in the daemon to run
external commands, and therefore very important.  These two patches
enhance these functions in useful ways.

(Note these are an internal interface which we can change at any
time).

The first patch adds variations which take a flags parameter, and
implements a useful flag (for dealing with parted).

The second patch stops command*() from eating stderr when the stderror
parameter is NULL and we're in debug mode.  Currently vital logging
and error information is being lost in certain circumstances, and this
patch resolves this.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
-------------- next part --------------
>From 1813b2df03ff6a4fe36d72c46827cf380185ca38 Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Fri, 6 Nov 2009 13:19:26 +0000
Subject: [PATCH 1/2] daemon: Add flags argument to command*() functions.

This adds new variations of the command*() functions which
take a 'flags' argument.  Currently the only flag available
is defined as follows:

 COMMAND_FLAG_FOLD_STDOUT_ON_STDERR: For broken external commands
 that send error messages to stdout (hello, parted) but that don't
 have any useful stdout information, use this flag to capture the
 error messages in the *stderror buffer.  If using this flag,
 you should pass stdoutput as NULL because nothing could ever be
 captured in that buffer.

This patch also adds some documentation for command*()
function.
---
 daemon/daemon.h   |   21 +++++++++++++++------
 daemon/guestfsd.c |   48 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 51 insertions(+), 18 deletions(-)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 86c6876..f082690 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -47,12 +47,21 @@ extern void sort_strings (char **argv, int len);
 extern void free_strings (char **argv);
 extern void free_stringslen (char **argv, int len);
 
-extern int command (char **stdoutput, char **stderror, const char *name, ...);
-extern int commandr (char **stdoutput, char **stderror, const char *name, ...);
-extern int commandv (char **stdoutput, char **stderror,
-                     char *const *argv);
-extern int commandrv (char **stdoutput, char **stderror,
-                      char const* const *argv);
+#define command(out,err,name,...) commandf((out),(err),0,(name),__VA_ARGS__)
+#define commandr(out,err,name,...) commandrf((out),(err),0,(name),__VA_ARGS__)
+#define commandv(out,err,argv) commandvf((out),(err),0,(argv))
+#define commandrv(out,err,argv) commandrvf((out),(err),0,(argv))
+
+#define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR 1
+
+extern int commandf (char **stdoutput, char **stderror, int flags,
+		     const char *name, ...);
+extern int commandrf (char **stdoutput, char **stderror, int flags,
+		      const char *name, ...);
+extern int commandvf (char **stdoutput, char **stderror, int flags,
+		      char *const *argv);
+extern int commandrvf (char **stdoutput, char **stderror, int flags,
+		       char const* const *argv);
 
 extern char **split_lines (char *str);
 
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 649a630..bf06c73 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -508,13 +508,11 @@ free_stringslen (char **argv, int len)
   free (argv);
 }
 
-/* This is a more sane version of 'system(3)' for running external
- * commands.  It uses fork/execvp, so we don't need to worry about
- * quoting of parameters, and it allows us to capture any error
- * messages in a buffer.
+/* Easy ways to run external commands.  For full documentation, see
+ * 'commandrvf' below.
  */
 int
-command (char **stdoutput, char **stderror, const char *name, ...)
+commandf (char **stdoutput, char **stderror, int flags, const char *name, ...)
 {
   va_list args;
   const char **argv;
@@ -548,7 +546,7 @@ command (char **stdoutput, char **stderror, const char
*name, ...)
 
   va_end (args);
 
-  r = commandv (stdoutput, stderror, (char **) argv);
+  r = commandvf (stdoutput, stderror, flags, (char **) argv);
 
   /* NB: Mustn't free the strings which are on the stack. */
   free (argv);
@@ -561,7 +559,7 @@ command (char **stdoutput, char **stderror, const char
*name, ...)
  * We still return -1 if there was some other error.
  */
 int
-commandr (char **stdoutput, char **stderror, const char *name, ...)
+commandrf (char **stdoutput, char **stderror, int flags, const char *name, ...)
 {
   va_list args;
   const char **argv;
@@ -595,7 +593,7 @@ commandr (char **stdoutput, char **stderror, const char
*name, ...)
 
   va_end (args);
 
-  r = commandrv (stdoutput, stderror, argv);
+  r = commandrvf (stdoutput, stderror, flags, argv);
 
   /* NB: Mustn't free the strings which are on the stack. */
   free (argv);
@@ -605,19 +603,42 @@ commandr (char **stdoutput, char **stderror, const char
*name, ...)
 
 /* Same as 'command', but passing an argv. */
 int
-commandv (char **stdoutput, char **stderror, char *const *argv)
+commandvf (char **stdoutput, char **stderror, int flags, char *const *argv)
 {
   int r;
 
-  r = commandrv (stdoutput, stderror, (void *) argv);
+  r = commandrvf (stdoutput, stderror, flags, (void *) argv);
   if (r == 0)
     return 0;
   else
     return -1;
 }
 
+/* This is a more sane version of 'system(3)' for running external
+ * commands.  It uses fork/execvp, so we don't need to worry about
+ * quoting of parameters, and it allows us to capture any error
+ * messages in a buffer.
+ *
+ * If stdoutput is not NULL, then *stdoutput will return the stdout
+ * of the command.
+ *
+ * If stderror is not NULL, then *stderror will return the stderr
+ * of the command.  If there is a final \n character, it is removed
+ * so you can use the error string directly in a call to
+ * reply_with_error.
+ *
+ * Flags:
+ *
+ * COMMAND_FLAG_FOLD_STDOUT_ON_STDERR: For broken external commands
+ * that send error messages to stdout (hello, parted) but that don't
+ * have any useful stdout information, use this flag to capture the
+ * error messages in the *stderror buffer.  If using this flag,
+ * you should pass stdoutput as NULL because nothing could ever be
+ * captured in that buffer.
+ */
 int
-commandrv (char **stdoutput, char **stderror, char const* const *argv)
+commandrvf (char **stdoutput, char **stderror, int flags,
+	    char const* const *argv)
 {
   int so_size = 0, se_size = 0;
   int so_fd[2], se_fd[2];
@@ -657,7 +678,10 @@ commandrv (char **stdoutput, char **stderror, char const*
const *argv)
     open ("/dev/null", O_RDONLY); /* Set stdin to /dev/null (ignore
failure) */
     close (so_fd[0]);
     close (se_fd[0]);
-    dup2 (so_fd[1], 1);
+    if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR))
+      dup2 (so_fd[1], 1);
+    else
+      dup2 (se_fd[1], 1);
     dup2 (se_fd[1], 2);
     close (so_fd[1]);
     close (se_fd[1]);
-- 
1.6.2.5

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
-------------- next part --------------
>From e924f1b87381b3f38b2ba98d4373ede8995ad82b Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Fri, 6 Nov 2009 17:27:02 +0000
Subject: [PATCH 2/2] daemon: Always reflect command stderr to stderr when
debugging.

When debugging (ie. LIBGUESTFS_DEBUG=1 & verbose flag set in daemon)
always reflect any stderr output from commands that we run to
stderr of the daemon, so it is visible.

Previously if stderror == NULL in command*, stderr output was
just eaten and discarded which meant useful error messages could
be lost.
---
 daemon/guestfsd.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index bf06c73..370eea8 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -38,6 +38,8 @@
 #include <printf.h>
 
 #include "c-ctype.h"
+#include "ignore-value.h"
+
 #include "daemon.h"
 
 static char *read_cmdline (void);
@@ -742,15 +744,20 @@ commandrvf (char **stdoutput, char **stderror, int flags,
       }
       if (r == 0) { FD_CLR (se_fd[0], &rset); quit++; }
 
-      if (r > 0 && stderror) {
-        se_size += r;
-        p = realloc (*stderror, se_size);
-        if (p == NULL) {
-          perror ("realloc");
-          goto quit;
-        }
-        *stderror = p;
-        memcpy (*stderror + se_size - r, buf, r);
+      if (r > 0) {
+	if (verbose)
+	  ignore_value (write (2, buf, r));
+
+	if (stderror) {
+	  se_size += r;
+	  p = realloc (*stderror, se_size);
+	  if (p == NULL) {
+	    perror ("realloc");
+	    goto quit;
+	  }
+	  *stderror = p;
+	  memcpy (*stderror + se_size - r, buf, r);
+	}
       }
     }
   }
-- 
1.6.2.5