Re: lossage due to non-blocking stderr

[Mark D. Baushke]

Hi Frank,

Many thanks for your description of the steps to reproduce the problem
in addition to your patch to client.c to help resolve it.

I have devised a test script that lets me reproduce the problem and I
converted it into a santiy.sh test.

I am planning to commit this patch to the cvs1-11-x-branch and then
merge it into the feature branch.

The test script reproduced the problem for cvs 1.11.x on these three
hosts:

   Redhat 7.3 GNU/Linux
   Gentoo 1.4.16 GNU/Linux
   Solaris 9

I also checked using Redhat 7.3 GNU/Linux and the same script shows a
problem with cvs 1.12.9.1, so I will also be merging the change forward
into the feature branch as well.

(Note: The problem does not seem to arise on FreeBSD 4.x systems.)

Does anyone see any problems with this patch?

        Thanks,   
        -- Mark

Index: src/ChangeLog
===================================================================
RCS file: /cvs/ccvs/src/ChangeLog,v
retrieving revision 1.2336.2.289
diff -u -p -r1.2336.2.289 ChangeLog
--- src/ChangeLog       23 Jun 2004 01:52:25 -0000      1.2336.2.289
+++ src/ChangeLog       17 Aug 2004 08:23:00 -0000
@@ -1,3 +1,14 @@
+2004-08-17  Mark D. Baushke  <mdb@cvshome.org>
+
+       * client.c (handle_m): Workaround to deal with stdio getting put
+       into non-blocking via redirection of stderr and interaction with
+       ssh on some platforms. On those boxes, stdio can put stdout
+       unexpectedly into non-blocking mode which may lead to fwrite() or
+       fflush() failing with EAGAIN, but cvs not checking for the error.
+       (Patch suggested by Frank Hemer <frank@hemer.org>.)
+
+       * sanity.sh (sshstdio): New test for non-blocking stdio via ssh.
+       
 2004-06-22  Derek Price  <derek@ximbiot.com>
 
        * wrapper.c: Add explicit "void" return type to "wrap_clean_fmt_str"
Index: src/client.c
===================================================================
RCS file: /cvs/ccvs/src/client.c,v
retrieving revision 1.318.4.20
diff -u -p -r1.318.4.20 client.c
--- src/client.c        26 Apr 2004 15:52:05 -0000      1.318.4.20
+++ src/client.c        17 Aug 2004 08:23:00 -0000
@@ -3068,6 +3068,9 @@ handle_m (args, len)
     char *args;
     int len;
 {
+    fd_set wfds;
+    int s;
+
     /* In the case where stdout and stderr point to the same place,
        fflushing stderr will make output happen in the correct order.
        Often stderr will be line-buffered and this won't be needed,
@@ -3075,6 +3078,11 @@ handle_m (args, len)
        based on being confused between default buffering between
        stdout and stderr.  But I'm not sure).  */
     fflush (stderr);
+    FD_ZERO (&wfds);
+    FD_SET (STDOUT_FILENO, &wfds);
+    s = select (STDOUT_FILENO+1, NULL, &wfds, NULL, NULL);
+    if (s < 1)
+        perror ("cannot write to stdout");
     fwrite (args, len, sizeof (*args), stdout);
     putc ('\n', stdout);
 }
Index: src/sanity.sh
===================================================================
RCS file: /cvs/ccvs/src/sanity.sh,v
retrieving revision 1.752.2.124
diff -u -p -r1.752.2.124 sanity.sh
--- src/sanity.sh       9 Jun 2004 13:17:46 -0000       1.752.2.124
+++ src/sanity.sh       17 Aug 2004 08:23:01 -0000
@@ -805,6 +805,7 @@ if test x"$*" = x; then
        # Repository Storage (RCS file format, CVS lock files, creating
        # a repository without "cvs init", &c).
        tests="${tests} crerepos rcs rcs2 rcs3 lockfiles backuprecover"
+       tests="${tests} sshstdio"
        # More history browsing, &c.
        tests="${tests} history"
        tests="${tests} big modes modes2 modes3 stamps"
@@ -19892,6 +19893,100 @@ done"
          rm -rf ${CVSROOT_DIRNAME}/first-dir
          ;;
 
+        sshstdio)
+          # CVS_RSH=ssh can have a problem with a non-blocking stdio
+          # in some cases. So, this test is all about testing :ext:
+          # with CVS_RSH=ssh
+
+          if $remote; then
+            CVS_RSH_save=$CVS_RSH
+            CVS_RSH=ssh; export CVS_RSH
+            # Work around X11Forarding by ssh
+            if [ -n "$DISPLAY" ]; then
+                DISPLAY_save=$DISPLAY
+                unset DISPLAY
+            fi
+  
+            if test -n "$remotehost"; then
+                  SSHSTDIO_ROOT=:ext:$remotehost${CVSROOT_DIRNAME}
+            else
+                  SSHSTDIO_ROOT=:ext:`hostname`:${CVSROOT_DIRNAME}
+            fi
+  
+            # If we're going to do remote testing, make sure 'ssh' works first.
+            host="`hostname`"
+            if test "x`${CVS_RSH} $host -n 'echo hi'`" != "xhi"; then
+              echo "ERROR: cannot test remote CVS, because \`${CVS_RSH} $host' 
fails." >&2
+              exit 1
+            fi
+  
+            mkdir sshstdio; cd sshstdio
+            dotest sshstdio-1 "${testcvs} -d ${SSHSTDIO_ROOT} -q co -l ." ''
+            mkdir first-dir
+            dotest sshstdio-2 "${testcvs} add first-dir" \
+  "Directory ${CVSROOT_DIRNAME}/first-dir added to the repository"
+            cd first-dir
+            
a='aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
+            
c='aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaacaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
+            # Generate 1024 lines of $a
+            cnt=0
+            echo $a > aaa
+            while [ $cnt -lt 5 ] ; do
+                cnt=`expr $cnt + 1` ;
+                mv aaa aaa.old
+                cat aaa.old aaa.old aaa.old aaa.old > aaa
+            done
+            dotest sshstdio-3 "${testcvs} -q add aaa" \
+"${PROG} add: use .${PROG} commit. to add this file permanently"
+          dotest sshstdio-4 "${testcvs} -q ci -mcreate aaa" \
+"RCS file: $CVSROOT_DIRNAME/first-dir/aaa,v
+done
+Checking in aaa;
+$CVSROOT_DIRNAME/first-dir/aaa,v  <--  aaa
+initial revision: 1\.1
+done"
+            # replace lines 1, 512, 513, 1024 with $c
+            sed 510q < aaa > aaa.old
+            (echo $c; cat aaa.old; echo $c; \
+             echo $c; cat aaa.old; echo $c) > aaa
+            dotest sshstdio-5 "${testcvs} -q ci -mmodify-it aaa" \
+"Checking in aaa;
+${CVSROOT_DIRNAME}/first-dir/aaa,v  <--  aaa
+new revision: 1\.2; previous revision: 1\.1
+done"
+            cat > wrapper.sh <<EOF
+#!${TESTSHELL}
+exec "\$@" 2>&1 < /dev/null | cat
+EOF
+            chmod +x wrapper.sh
+            ./wrapper.sh \
+             ${testcvs} -z5 -Q diff --side-by-side -W 500 -r 1.1 -r 1.2 \
+               aaa >wrapper.dif
+  
+            ${testcvs} -z5 -Q diff --side-by-side -W 500 -r 1.1 -r 1.2 \
+               aaa >good.dif
+  
+            dotest sshstdio-6 "cmp wrapper.dif good.dif" ""
+  
+            CVS_RSH=$CVS_RSH_save; export CVS_RSH
+            if [ -n "$DISPLAY" ]; then
+                DISPLAY=$DISPLAY_save; export DISPLAY
+            fi
+
+            if $keep; then
+                echo Keeping ${TESTDIR} and exiting due to --keep
+                exit 0
+            fi
+  
+            cd ../..
+            rm -r sshstdio
+            rm -rf ${CVSROOT_DIRNAME}/first-dir
+          else
+            :  # nothing to do unless $remote
+          fi
+          ;;
+
+
        history)
          # CVSROOT/history tests:
          # history: various "cvs history" invocations