Libguestfs - Dec 2012 - [PATCH 1/2] daemon: NFC Use symbolic names in commandrvf

Improve readability of commandrvf() by replacing bare int values for
file descriptors with their symbolic names STD{IN,OUT,ERR}_FILENO.

Also add PIPE_READ and PIPE_WRITE for referencing relevant ends of a pipe.
---
 daemon/guestfsd.c | 79 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 37 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 5c84849..4b3dd5f 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -66,6 +66,10 @@ static char *read_cmdline (void);
 # define O_CLOEXEC 0
 #endif
 
+/* For improved readability dealing with pipe arrays */
+#define PIPE_READ 0
+#define PIPE_WRITE 1
+
 /* If root device is an ext2 filesystem, this is the major and minor.
  * This is so we can ignore this device from the point of view of the
  * user, eg. in guestfs_list_devices and many other places.
@@ -834,22 +838,22 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     signal (SIGPIPE, SIG_DFL);
     close (0);
     if (flag_copy_stdin) {
-      dup2 (stdin_fd[0], 0);
-      close (stdin_fd[0]);
-      close (stdin_fd[1]);
+      dup2 (stdin_fd[PIPE_READ], STDIN_FILENO);
+      close (stdin_fd[PIPE_READ]);
+      close (stdin_fd[PIPE_WRITE]);
     } else {
       /* Set stdin to /dev/null (ignore failure) */
       ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));
     }
-    close (so_fd[0]);
-    close (se_fd[0]);
+    close (so_fd[PIPE_READ]);
+    close (se_fd[PIPE_READ]);
     if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR))
-      dup2 (so_fd[1], 1);
+      dup2 (so_fd[PIPE_WRITE], STDOUT_FILENO);
     else
-      dup2 (se_fd[1], 1);
-    dup2 (se_fd[1], 2);
-    close (so_fd[1]);
-    close (se_fd[1]);
+      dup2 (se_fd[PIPE_WRITE], STDOUT_FILENO);
+    dup2 (se_fd[PIPE_WRITE], STDERR_FILENO);
+    close (so_fd[PIPE_WRITE]);
+    close (se_fd[PIPE_WRITE]);
 
     execvp (argv[0], (void *) argv);
     perror (argv[0]);
@@ -866,15 +870,15 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     }
 
     if (stdin_pid == 0) {       /* Child process copying stdin. */
-      close (so_fd[0]);
-      close (so_fd[1]);
-      close (se_fd[0]);
-      close (se_fd[1]);
+      close (so_fd[PIPE_READ]);
+      close (so_fd[PIPE_WRITE]);
+      close (se_fd[PIPE_READ]);
+      close (se_fd[PIPE_WRITE]);
 
-      close (1);
-      dup2 (stdin_fd[1], 1);
-      close (stdin_fd[0]);
-      close (stdin_fd[1]);
+      close (STDOUT_FILENO);
+      dup2 (stdin_fd[PIPE_WRITE], STDOUT_FILENO);
+      close (stdin_fd[PIPE_READ]);
+      close (stdin_fd[PIPE_WRITE]);
 
       if (chroot (sysroot) == -1) {
         perror ("chroot");
@@ -884,7 +888,7 @@ commandrvf (char **stdoutput, char **stderror, int flags,
       ssize_t n;
       char buffer[BUFSIZ];
       while ((n = read (fd, buffer, sizeof buffer)) > 0) {
-        if (xwrite (1, buffer, n) == -1)
+        if (xwrite (STDOUT_FILENO, buffer, n) == -1)
           /* EPIPE error indicates the command process has exited
            * early.  If the command process fails that will be caught
            * by the daemon, and if not, then it's not an error.
@@ -906,23 +910,24 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     }
 
     close (fd);
-    close (stdin_fd[0]);
-    close (stdin_fd[1]);
+    close (stdin_fd[PIPE_READ]);
+    close (stdin_fd[PIPE_WRITE]);
   }
 
   /* Parent process. */
-  close (so_fd[1]);
-  close (se_fd[1]);
+  close (so_fd[PIPE_WRITE]);
+  close (se_fd[PIPE_WRITE]);
 
   FD_ZERO (&rset);
-  FD_SET (so_fd[0], &rset);
-  FD_SET (se_fd[0], &rset);
+  FD_SET (so_fd[PIPE_READ], &rset);
+  FD_SET (se_fd[PIPE_READ], &rset);
 
   quit = 0;
   while (quit < 2) {
   again:
     rset2 = rset;
-    r = select (MAX (so_fd[0], se_fd[0]) + 1, &rset2, NULL, NULL, NULL);
+    r = select (MAX (so_fd[PIPE_READ], se_fd[PIPE_READ]) + 1, &rset2,
+                NULL, NULL, NULL);
     if (r == -1) {
       if (errno == EINTR)
         goto again;
@@ -943,20 +948,20 @@ commandrvf (char **stdoutput, char **stderror, int flags,
         *stderror = strdup ("error running external command, "
                             "see debug output for details");
       }
-      close (so_fd[0]);
-      close (se_fd[0]);
+      close (so_fd[PIPE_READ]);
+      close (se_fd[PIPE_READ]);
       waitpid (pid, NULL, 0);
       if (stdin_pid >= 0) waitpid (stdin_pid, NULL, 0);
       return -1;
     }
 
-    if (FD_ISSET (so_fd[0], &rset2)) { /* something on stdout */
-      r = read (so_fd[0], buf, sizeof buf);
+    if (FD_ISSET (so_fd[PIPE_READ], &rset2)) { /* something on stdout */
+      r = read (so_fd[PIPE_READ], buf, sizeof buf);
       if (r == -1) {
         perror ("read");
         goto quit;
       }
-      if (r == 0) { FD_CLR (so_fd[0], &rset); quit++; }
+      if (r == 0) { FD_CLR (so_fd[PIPE_READ], &rset); quit++; }
 
       if (r > 0 && stdoutput) {
         so_size += r;
@@ -970,17 +975,17 @@ commandrvf (char **stdoutput, char **stderror, int flags,
       }
     }
 
-    if (FD_ISSET (se_fd[0], &rset2)) { /* something on stderr */
-      r = read (se_fd[0], buf, sizeof buf);
+    if (FD_ISSET (se_fd[PIPE_READ], &rset2)) { /* something on stderr */
+      r = read (se_fd[PIPE_READ], buf, sizeof buf);
       if (r == -1) {
         perror ("read");
         goto quit;
       }
-      if (r == 0) { FD_CLR (se_fd[0], &rset); quit++; }
+      if (r == 0) { FD_CLR (se_fd[PIPE_READ], &rset); quit++; }
 
       if (r > 0) {
         if (verbose)
-          ignore_value (write (2, buf, r));
+          ignore_value (write (STDERR_FILENO, buf, r));
 
         if (stderror) {
           se_size += r;
@@ -996,8 +1001,8 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     }
   }
 
-  close (so_fd[0]);
-  close (se_fd[0]);
+  close (so_fd[PIPE_READ]);
+  close (se_fd[PIPE_READ]);
 
   /* Make sure the output buffers are \0-terminated.  Also remove any
    * trailing \n characters from the error buffer (not from stdout).
-- 
1.7.11.7

Currently the code is doing a redundant fork when passed the
COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN flag. The additional sub-process does a
chroot() which has no effect because all file handles are already open at that
point, then simply copies its input to its output.

This change simply replaces the above with a dup2 of the passed file handle to
STDIN of the command process.
---
 daemon/guestfsd.c | 89 +++++--------------------------------------------------
 1 file changed, 7 insertions(+), 82 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 4b3dd5f..7e83a2a 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -789,8 +789,8 @@ commandrvf (char **stdoutput, char **stderror, int flags,
   size_t so_size = 0, se_size = 0;
   int so_fd[2], se_fd[2];
   int flag_copy_stdin = flags & COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN;
-  int stdin_fd[2] = { -1, -1 };
-  pid_t pid, stdin_pid = -1;
+  int flag_copy_fd = flags & COMMAND_FLAG_FD_MASK;
+  pid_t pid = -1;
   int r, quit, i;
   fd_set rset, rset2;
   char buf[256];
@@ -820,13 +820,6 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     abort ();
   }
 
-  if (flag_copy_stdin) {
-    if (pipe (stdin_fd) == -1) {
-      error (0, errno, "pipe");
-      abort ();
-    }
-  }
-
   pid = fork ();
   if (pid == -1) {
     error (0, errno, "fork");
@@ -838,9 +831,7 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     signal (SIGPIPE, SIG_DFL);
     close (0);
     if (flag_copy_stdin) {
-      dup2 (stdin_fd[PIPE_READ], STDIN_FILENO);
-      close (stdin_fd[PIPE_READ]);
-      close (stdin_fd[PIPE_WRITE]);
+      dup2 (flag_copy_fd, STDIN_FILENO);
     } else {
       /* Set stdin to /dev/null (ignore failure) */
       ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));
@@ -860,60 +851,6 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     _exit (EXIT_FAILURE);
   }
 
-  if (flag_copy_stdin) {
-    int fd = flags & COMMAND_FLAG_FD_MASK;
-
-    stdin_pid = fork ();
-    if (stdin_pid == -1) {
-      error (0, errno, "fork");
-      abort ();
-    }
-
-    if (stdin_pid == 0) {       /* Child process copying stdin. */
-      close (so_fd[PIPE_READ]);
-      close (so_fd[PIPE_WRITE]);
-      close (se_fd[PIPE_READ]);
-      close (se_fd[PIPE_WRITE]);
-
-      close (STDOUT_FILENO);
-      dup2 (stdin_fd[PIPE_WRITE], STDOUT_FILENO);
-      close (stdin_fd[PIPE_READ]);
-      close (stdin_fd[PIPE_WRITE]);
-
-      if (chroot (sysroot) == -1) {
-        perror ("chroot");
-        _exit (EXIT_FAILURE);
-      }
-
-      ssize_t n;
-      char buffer[BUFSIZ];
-      while ((n = read (fd, buffer, sizeof buffer)) > 0) {
-        if (xwrite (STDOUT_FILENO, buffer, n) == -1)
-          /* EPIPE error indicates the command process has exited
-           * early.  If the command process fails that will be caught
-           * by the daemon, and if not, then it's not an error.
-           */
-          _exit (errno == EPIPE ? EXIT_SUCCESS : EXIT_FAILURE);
-      }
-
-      if (n == -1) {
-        perror ("read");
-        _exit (EXIT_FAILURE);
-      }
-
-      if (close (fd) == -1) {
-        perror ("close");
-        _exit (EXIT_FAILURE);
-      }
-
-      _exit (EXIT_SUCCESS);
-    }
-
-    close (fd);
-    close (stdin_fd[PIPE_READ]);
-    close (stdin_fd[PIPE_WRITE]);
-  }
-
   /* Parent process. */
   close (so_fd[PIPE_WRITE]);
   close (se_fd[PIPE_WRITE]);
@@ -950,8 +887,8 @@ commandrvf (char **stdoutput, char **stderror, int flags,
       }
       close (so_fd[PIPE_READ]);
       close (se_fd[PIPE_READ]);
+      if (flag_copy_stdin) close (flag_copy_fd);
       waitpid (pid, NULL, 0);
-      if (stdin_pid >= 0) waitpid (stdin_pid, NULL, 0);
       return -1;
     }
 
@@ -1033,21 +970,9 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     }
   }
 
-  if (flag_copy_stdin) {
-    /* Check copy process didn't fail. */
-    if (waitpid (stdin_pid, &r, 0) != stdin_pid) {
-      perror ("waitpid");
-      kill (pid, 9);
-      waitpid (pid, NULL, 0);
-      return -1;
-    }
-
-    if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
-      fprintf (stderr, "failed copying from input file, see earlier
messages (r = %d)\n", r);
-      kill (pid, 9);
-      waitpid (pid, NULL, 0);
-      return -1;
-    }
+  if (flag_copy_stdin && close (flag_copy_fd) == -1) {
+    perror ("close");
+    return -1;
   }
 
   /* Get the exit status of the command. */
-- 
1.7.11.7

On Thu, Dec 13, 2012 at 03:22:38PM +0000, Matthew Booth
wrote:
> Improve readability of commandrvf() by replacing bare int values for
> file descriptors with their symbolic names STD{IN,OUT,ERR}_FILENO.
> 
> Also add PIPE_READ and PIPE_WRITE for referencing relevant ends of a pipe.
Thanks -- applied, will push it shortly.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora