Re: [Bug-gnulib] closeout doesn't always close: which is better: always or never?

[Jim Meyering]

Bruno Haible <address@hidden> wrote:
> Jim Meyering wrote:
>> Is there any reason to call fclose rather than just fflush?
>
> In Linux, at least (see linux/fs/open.c), close() can return an error if
> the file system dependent f_op->flush function fails. close() is the only
> system call to use f_op->flush; therefore on file systems that have a
> 'flush' operation (NFS and Coda) close() can fail even when the previous
> write() system calls from fflush() have succeeded. The obvious reason is
> that for these filesystems, there OS maintains a cache into which write()
> writes, and this cache is not emptied until close().

Thanks for investigating that!

That makes it clear to me that close_stdout must always close
standard output.

Here's the patch I'm checking in to coreutils.
The only additional change I had to make was to sort,
in order to make it avoid closing stdout twice.

I'll use it locally for a day or two before putting it in gnulib.

        Ensure that no close failure goes unreported.
        * closeout.c (close_stdout): Always close stdout.  I.e., don't
        return early when it seems there's nothing to flush.
        Don't include __fpending.h.

        * src/sort.c (xfclose): Don't close stdout here (just flush it),
        since close_stdout now closes stdout unconditionally.

Index: lib/closeout.c
===================================================================
RCS file: /fetish/cu/lib/closeout.c,v
retrieving revision 1.15
diff -u -p -r1.15 closeout.c
--- lib/closeout.c      4 Oct 2004 20:18:43 -0000       1.15
+++ lib/closeout.c      6 Nov 2004 22:20:23 -0000
@@ -32,7 +32,6 @@
 #include "error.h"
 #include "exitfail.h"
 #include "quotearg.h"
-#include "__fpending.h"
 
 #if USE_UNLOCKED_IO
 # include "unlocked-io.h"
@@ -49,7 +48,7 @@ close_stdout_set_file_name (const char *
 }
 
 /* Close standard output, exiting with status 'exit_failure' on failure.
-   If a program writes *anything* to stdout, that program should `fflush'
+   If a program writes *anything* to stdout, that program should close
    stdout and make sure that it succeeds before exiting.  Otherwise,
    suppose that you go to the extreme of checking the return status
    of every function that does an explicit write to stdout.  The last
@@ -57,11 +56,9 @@ close_stdout_set_file_name (const char *
    the fclose(stdout) could still fail (due e.g., to a disk full error)
    when it tries to write out that buffered data.  Thus, you would be
    left with an incomplete output file and the offending program would
-   exit successfully.
-
-   FIXME: note the fflush suggested above is implicit in the fclose
-   we actually do below.  Consider doing only the fflush and/or using
-   setvbuf to inhibit buffering.
+   exit successfully.  Even calling fflush is not always sufficient,
+   since some file systems (NFS and CODA) buffer written/flushed data
+   until an actual close call.
 
    Besides, it's wasteful to check the return value from every call
    that writes to stdout -- just let the internal stream state record
@@ -76,11 +73,6 @@ close_stdout (void)
 {
   int e = ferror (stdout) ? 0 : -1;
 
-  /* If the stream's error bit is clear and there is nothing to flush,
-     then return right away.  */
-  if (e && __fpending (stdout) == 0)
-    return;
-
   if (fclose (stdout) != 0)
     e = errno;
 
Index: src/sort.c
===================================================================
RCS file: /fetish/cu/src/sort.c,v
retrieving revision 1.294
diff -u -p -r1.294 sort.c
--- src/sort.c  5 Nov 2004 23:02:09 -0000       1.294
+++ src/sort.c  6 Nov 2004 20:52:34 -0000
@@ -480,6 +480,12 @@ xfclose (FILE *fp, char const *file)
       if (feof (fp))
        clearerr (fp);
     }
+  else if (fp == stdout)
+    {
+      /* Don't close stdout just yet.  close_stdout does that.  */
+      if (fflush (fp) != 0)
+       die (_("fflush failed"), file);
+    }
   else
     {
       if (fclose (fp) != 0)