Re: Help with create_pipe_bidi

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Bruno Haible on 7/19/2009 4:53 AM:
> I've committed this. The test now passes on mingw (and of course also on
> Linux and Cygwin).

I'm committing this followup, which fixes a comment rendered stale by your
previous commit, adds the same cautionary comment that you are proposing
for pipe-filter, and enhances the testsuite to 1) attempt a larger read in
the child while rearranging calls in the parent to avoid deadlock in that
case, and 2) always report ASSERT failures to the original stderr,
regardless of what the test did to fd 2.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpkZzAACgkQ84KuGfSFAYDmvwCdFSqIbhXEZ9mGs4BRYPZAfEoZ
LpgAoIxPrrtzhqv+okhKjjJB3eYYqfK4
=nMVB
-----END PGP SIGNATURE-----

From deb58d481fc8dc345e463002c7087a08a4f1d5f0 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Mon, 20 Jul 2009 06:41:01 -0600
Subject: [PATCH] test-pipe: make a bit more robust.

* tests/test-pipe.c (myerr): Allow error messages regardless of
what we do to stderr.
(test_pipe): Rearrange to avoid deadlock.
(child_main): Try a larger read, to ensure we avoided deadlock.
* lib/pipe.c (create_pipe) [_WIN32]: Fix comment.
* lib/pipe.h (create_pipe_bidi): Document potential for deadlock
if misused.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog         |   11 +++++++++++
 lib/pipe.c        |    4 +---
 lib/pipe.h        |   12 ++++++++++++
 tests/test-pipe.c |   39 ++++++++++++++++++++++++++++-----------
 4 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 9c78852..ea5f5fe 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2009-07-20  Eric Blake  <address@hidden>
+
+       test-pipe: make a bit more robust.
+       * tests/test-pipe.c (myerr): Allow error messages regardless of
+       what we do to stderr.
+       (test_pipe): Rearrange to avoid deadlock.
+       (child_main): Try a larger read, to ensure we avoided deadlock.
+       * lib/pipe.c (create_pipe) [_WIN32]: Fix comment.
+       * lib/pipe.h (create_pipe_bidi): Document potential for deadlock
+       if misused.
+
 2009-07-19  Jim Meyering  <address@hidden>

        fts: avoid false-positive cycle-detection
diff --git a/lib/pipe.c b/lib/pipe.c
index 5d91590..daf7f7f 100644
--- a/lib/pipe.c
+++ b/lib/pipe.c
@@ -185,9 +185,7 @@ create_pipe (const char *progname,
                      && close (stdoutfd) >= 0)))))
     /* The child process doesn't inherit ifd[0], ifd[1], ofd[0], ofd[1],
        but it inherits all open()ed or dup2()ed file handles (which is what
-       we want in the case of STD*_FILENO) and also orig_stdin,
-       orig_stdout, orig_stderr (which is not explicitly wanted but
-       harmless).  */
+       we want in the case of STD*_FILENO).  */
     /* Use spawnvpe and pass the environment explicitly.  This is needed if
        the program has modified the environment using putenv() or [un]setenv().
        On Windows, programs have two environments, one in the "environment
diff --git a/lib/pipe.h b/lib/pipe.h
index 03f2c32..082f687 100644
--- a/lib/pipe.h
+++ b/lib/pipe.h
@@ -109,6 +109,18 @@ extern pid_t create_pipe_in (const char *progname,
  *
  * Note: When writing to a child process, it is useful to ignore the SIGPIPE
  * signal and the EPIPE error code.
+ *
+ * Note: The parent process must be careful to avoid deadlock.
+ * 1) If you write more than PIPE_MAX bytes or, more generally, if you write
+ *    more bytes than the subprocess can handle at once, the subprocess
+ *    may write its data and wait on you to read it, but you are currently
+ *    busy writing.
+ * 2) When you don't know ahead of time how many bytes the subprocess
+ *    will produce, the usual technique of calling read (fd, buf, BUFSIZ)
+ *    with a fixed BUFSIZ will, on Linux 2.2.17 and on BSD systems, cause
+ *    the read() call to block until *all* of the buffer has been filled.
+ *    But the subprocess cannot produce more data until you gave it more
+ *    input.  But you are currently busy reading from it.
  */
 extern pid_t create_pipe_bidi (const char *progname,
                               const char *prog_path, char **prog_argv,
diff --git a/tests/test-pipe.c b/tests/test-pipe.c
index 023f403..e28fae7 100644
--- a/tests/test-pipe.c
+++ b/tests/test-pipe.c
@@ -34,15 +34,19 @@
 #include <string.h>

 /* Depending on arguments, this test intentionally closes stderr or
-   starts life with stderr closed.  So, the error messages might not
-   always print, but at least the exit status will be reliable.  */
+   starts life with stderr closed.  So, we arrange to have fd 10
+   (outside the range of interesting fd's during the test) set up to
+   duplicate the original stderr.  */
+
+static FILE *myerr;
+
 #define ASSERT(expr) \
   do                                                                         \
     {                                                                        \
       if (!(expr))                                                           \
         {                                                                    \
-          fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \
-          fflush (stderr);                                                   \
+          fprintf (myerr, "%s:%d: assertion failed\n", __FILE__, __LINE__);  \
+          fflush (myerr);                                                    \
           abort ();                                                          \
         }                                                                    \
     }                                                                        \
@@ -52,7 +56,7 @@
 static int
 child_main (int argc, char *argv[])
 {
-  char buffer[1];
+  char buffer[2] = { 's', 't' };
   int fd;

   ASSERT (argc == 3);
@@ -61,7 +65,7 @@ child_main (int argc, char *argv[])
      fd 2 should be closed iff the argument is 1.  Check that no other file
      descriptors leaked.  */

-  ASSERT (read (STDIN_FILENO, buffer, 1) == 1);
+  ASSERT (read (STDIN_FILENO, buffer, 2) == 1);

   buffer[0]++;
   ASSERT (write (STDOUT_FILENO, buffer, 1) == 1);
@@ -141,6 +145,7 @@ test_pipe (const char *argv0, bool stderr_closed)

   /* Push child's input.  */
   ASSERT (write (fd[1], buffer, 1) == 1);
+  ASSERT (close (fd[1]) == 0);

   /* Get child's output.  */
   ASSERT (read (fd[0], buffer, 2) == 1);
@@ -148,7 +153,6 @@ test_pipe (const char *argv0, bool stderr_closed)
   /* Wait for child.  */
   ASSERT (wait_subprocess (pid, argv0, true, false, true, true, NULL) == 0);
   ASSERT (close (fd[0]) == 0);
-  ASSERT (close (fd[1]) == 0);

   /* Check the result.  */
   ASSERT (buffer[0] == 'b');
@@ -215,9 +219,22 @@ parent_main (int argc, char *argv[])
 int
 main (int argc, char *argv[])
 {
-  ASSERT (argc >= 2);
+  if (argc < 2)
+    {
+      fprintf (stderr, "%s: need arguments\n", argv[0]);
+      return 2;
+    }
   if (strcmp (argv[1], "child") == 0)
-    return child_main (argc, argv);
-  else
-    return parent_main (argc, argv);
+    {
+      /* fd 2 might be closed, but fd 10 is the original stderr.  */
+      myerr = fdopen (10, "w");
+      if (!myerr)
+       return 2;
+      return child_main (argc, argv);
+    }
+  /* We might close fd 2 later, so save it in fd 10.  */
+  if (dup2 (STDERR_FILENO, 10) != 10
+      || (myerr = fdopen (10, "w")) == NULL)
+    return 2;
+  return parent_main (argc, argv);
 }
-- 
1.6.3.3.334.g916e1