Re: yesno module consumes too much input

[Eric Blake]

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

According to Bruno Haible on 8/18/2007 11:38 AM:
> Hi Eric,
> 
>> How about the following patch?
>>      * lib/closein.c (close_stdin_only): New function.
>>      (close_stdin): Make closing stdout optional.
>>      * lib/closein.h (close_stdin_only): Add declaration.
> 
> This API of closein.h can easily lead to bugs

Good catch.

> 
> To make this API more easy to understand, how about this?
>   1) Rename the function 'close_stdin' to 'close_stdin_stdout',
>   2) Add a function 'close_stdin', which closes stdin only. In the
>      implementation, arrange that when 'close_stdin' and 'close_stdin_stdout'
>      are both called, the stdin part is done only once.
>   3) Don't add a 'close_stdin_only' setter.

As in the following?

Note that I still have a question on behavior.  There is no way to query
what has previously been registered via atexit, so in this sequence:

atexit(close_stdout);
atexit(close_stdin);

results in calling close_stdin first.  From there, if close_stdin detects
an error, there is the potential for stdout/stderr to be unflushed (and
thus output data lost) when close_stdin calls _exit.  Whereas:

atexit(close_stdin_stdout);

flushes stdout before calling _exit.

About all I could think of that would avoid this problem is by adding a
new API, such as close_*_onexit(), used in place of the current
atexit(close_*), such that closein and closeout could communicate via
global variables which of the various close_* functions have been
registered, and thus do all the cleanup and flushing via a single atexit
handler.  But that is more invasive, as there are already a number of
programs using atexit(close_stdout).

2007-08-18  Eric Blake  <address@hidden>

        Ensure yesno does not consume too much seekable stdin.
        * modules/yesno (Depends-on): Add closein.
        * lib/closein.c (close_stdin_stdout): New function.
        (close_stdin): No longer close stdout.
        * lib/closein.h (close_stdin_stdout): Add declaration.
        * NEWS: Document API change.
        * lib/yesno.c (yesno): Register to call close_stdin at exit.
        * modules/yesno-tests (Files): New module.
        * tests/test-yesno.c (main): New file.
        * tests/test-yesno.sh: Likewise.

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

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

iD8DBQFGx1Kc84KuGfSFAYARAufOAJ40pm1AMqb2XdIc25sBulewkrQHLACgyaGT
KkW5ISIfL1qitoRP1ndiK0k=
=DfIc
-----END PGP SIGNATURE-----

Index: NEWS
===================================================================
RCS file: /sources/gnulib/gnulib/NEWS,v
retrieving revision 1.29
diff -u -p -r1.29 NEWS
--- NEWS        18 Aug 2007 07:16:53 -0000      1.29
+++ NEWS        18 Aug 2007 18:55:44 -0000
@@ -6,6 +6,9 @@ User visible incompatible changes
 
 Date        Modules         Changes
 
+2007-08-18  closein         close_stdin now leaves stdout alone.  Use the
+                            new close_stdin_stdout for the former behavior.
+
 2007-08-18  idcache         Now provides prototypes in "idcache.h".
 
 2007-08-10  xstrtol         The STRTOL_FATAL_ERROR macro is removed.
Index: lib/closein.c
===================================================================
RCS file: /sources/gnulib/gnulib/lib/closein.c,v
retrieving revision 1.3
diff -u -p -r1.3 closein.c
--- lib/closein.c       27 Apr 2007 17:14:40 -0000      1.3
+++ lib/closein.c       18 Aug 2007 18:55:44 -0000
@@ -45,9 +45,42 @@ close_stdin_set_file_name (const char *f
   file_name = file;
 }
 
+
+/* Implementation for both close_stdin and close_stdin_stdout;
+   close_stdout is called if FLAG is set.  */
+static void
+close_stdin_common (bool flag)
+{
+  bool fail = false;
+
+  /* Only attempt flush if stdin is seekable, as fflush is entitled to
+     fail on non-seekable streams.  */
+  if (fseeko (stdin, 0, SEEK_CUR) == 0 && fflush (stdin) != 0)
+    fail = true;
+  if (close_stream (stdin) != 0)
+    fail = true;
+  if (fail)
+    {
+      /* Report failure, but defer exit until after closing stdout,
+        since the failure report should still be flushed.  */
+      char const *close_error = _("error closing file");
+      if (file_name)
+       error (0, errno, "%s: %s", quotearg_colon (file_name),
+              close_error);
+      else
+       error (0, errno, "%s", close_error);
+    }
+
+  if (flag)
+    close_stdout ();
+
+  if (fail)
+    _exit (exit_failure);
+}
+
 /* Close standard input, rewinding any unused input if stdin is
    seekable.  On error, issue a diagnostic and _exit with status
-   'exit_failure'.  Then call close_stdout.
+   'exit_failure'.
 
    Most programs can get by with close_stdout.  close_stdin is only
    needed when a program wants to guarantee that partially read input
@@ -78,28 +111,13 @@ close_stdin_set_file_name (const char *f
 void
 close_stdin (void)
 {
-  bool fail = false;
-
-  /* Only attempt flush if stdin is seekable, as fflush is entitled to
-     fail on non-seekable streams.  */
-  if (fseeko (stdin, 0, SEEK_CUR) == 0 && fflush (stdin) != 0)
-    fail = true;
-  if (close_stream (stdin) != 0)
-    fail = true;
-  if (fail)
-    {
-      /* Report failure, but defer exit until after closing stdout,
-        since the failure report should still be flushed.  */
-      char const *close_error = _("error closing file");
-      if (file_name)
-       error (0, errno, "%s: %s", quotearg_colon (file_name),
-              close_error);
-      else
-       error (0, errno, "%s", close_error);
-    }
+  close_stdin_common (false);
+}
 
-  close_stdout ();
+/* Like close_stdin, except also call close_stdout.  */
 
-  if (fail)
-    _exit (exit_failure);
+void
+close_stdin_stdout (void)
+{
+  close_stdin_common (true);
 }
Index: lib/closein.h
===================================================================
RCS file: /sources/gnulib/gnulib/lib/closein.h,v
retrieving revision 1.1
diff -u -p -r1.1 closein.h
--- lib/closein.h       12 Apr 2007 16:11:40 -0000      1.1
+++ lib/closein.h       18 Aug 2007 18:55:44 -0000
@@ -25,6 +25,7 @@ extern "C" {
 
 void close_stdin_set_file_name (const char *file);
 void close_stdin (void);
+void close_stdin_stdout (void);
 
 # ifdef __cplusplus
 }
Index: lib/yesno.c
===================================================================
RCS file: /sources/gnulib/gnulib/lib/yesno.c,v
retrieving revision 1.17
diff -u -p -r1.17 yesno.c
--- lib/yesno.c 14 Dec 2006 18:47:36 -0000      1.17
+++ lib/yesno.c 18 Aug 2007 18:55:44 -0000
@@ -1,6 +1,6 @@
 /* yesno.c -- read a yes/no response from stdin
 
-   Copyright (C) 1990, 1998, 2001, 2003, 2004, 2005, 2006 Free
+   Copyright (C) 1990, 1998, 2001, 2003, 2004, 2005, 2006, 2007 Free
    Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
@@ -24,14 +24,17 @@
 #include <stdlib.h>
 #include <stdio.h>
 
+#include "closein.h"
+
 #if ENABLE_NLS
 # include "getline.h"
 #endif
 
-/* Return true if we read an affirmative line from standard input.  */
 
 extern int rpmatch (char const *response);
 
+/* Return true if we read an affirmative line from standard input.  */
+
 bool
 yesno (void)
 {
@@ -60,5 +63,17 @@ yesno (void)
     c = getchar ();
 #endif
 
+  /* Assume that yesno is the only client of stdin for a given
+     program.  Thus, if we get here, we need to clean up stdin on
+     exit.  */
+  {
+    static bool init;
+    if (!init)
+      {
+       init = true;
+       atexit (close_stdin);
+      }
+  }
+
   return yes;
 }
Index: modules/yesno
===================================================================
RCS file: /sources/gnulib/gnulib/modules/yesno,v
retrieving revision 1.13
diff -u -p -r1.13 yesno
--- modules/yesno       13 Oct 2006 12:40:23 -0000      1.13
+++ modules/yesno       18 Aug 2007 18:55:44 -0000
@@ -8,6 +8,7 @@ lib/yesno.h
 m4/yesno.m4
 
 Depends-on:
+closein
 getline
 rpmatch
 stdbool
Index: modules/yesno-tests
===================================================================
RCS file: modules/yesno-tests
diff -N modules/yesno-tests
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ modules/yesno-tests 18 Aug 2007 18:55:44 -0000
@@ -0,0 +1,14 @@
+Files:
+tests/test-yesno.c
+tests/test-yesno.sh
+
+Depends-on:
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-yesno.sh
+TESTS_ENVIRONMENT += EXEEXT='@EXEEXT@'
+check_PROGRAMS += test-yesno
+EXTRA_DIST += test-yesno.sh
+test_yesno_LDADD = $(LDADD) @LIBINTL@
Index: tests/test-yesno.c
===================================================================
RCS file: tests/test-yesno.c
diff -N tests/test-yesno.c
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ tests/test-yesno.c  18 Aug 2007 18:55:44 -0000
@@ -0,0 +1,37 @@
+/* Test of yesno module.
+   Copyright (C) 2007 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "yesno.h"
+
+char *program_name;
+
+int
+main (int argc, char **argv)
+{
+  int i = 1;
+  program_name = argv[0];
+  if (1 < argc)
+    i = atoi (argv[1]);
+  while (i--)
+    puts (yesno () ? "Y" : "N");
+  return 0;
+}
Index: tests/test-yesno.sh
===================================================================
RCS file: tests/test-yesno.sh
diff -N tests/test-yesno.sh
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ tests/test-yesno.sh 18 Aug 2007 18:55:44 -0000
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+tmpfiles=
+trap 'rm -fr $tmpfiles' 1 2 3 15
+
+p=t-yesno-
+tmpfiles="${p}in.tmp ${p}xout.tmp ${p}out.tmp"
+
+# Test with seekable stdin; followon process must see remaining data
+cat <<EOF > ${p}in.tmp
+nnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnn
+yn
+y
+n
+EOF
+cat <<EOF > ${p}xout.tmp
+N
+Y
+Y
+n
+EOF
+(./test-yesno${EXEEXT}; ./test-yesno${EXEEXT} 2; cat) \
+  < ${p}in.tmp > ${p}out.tmp || exit 1
+cmp ${p}xout.tmp ${p}out.tmp || exit 1
+
+(./test-yesno${EXEEXT} 2; ./test-yesno${EXEEXT}; cat) \
+  < ${p}in.tmp > ${p}out.tmp || exit 1
+cmp ${p}xout.tmp ${p}out.tmp || exit 1
+
+# Test for lack of error on pipe
+cat <<EOF > ${p}xout.tmp
+Y
+N
+EOF
+echo yes | ./test-yesno${EXEEXT} 2 > ${p}out.tmp || exit 1
+cmp ${p}xout.tmp ${p}out.tmp || exit 1
+
+# Test for lack of error when nothing is read
+cat <<EOF > ${p}xout.tmp
+N
+EOF
+./test-yesno${EXEEXT} </dev/null > ${p}out.tmp || exit 1
+cmp ${p}xout.tmp ${p}out.tmp || exit 1
+
+# Cleanup
+rm -fr $tmpfiles
+
+exit 0