Re: Ping: [PATCH] grep testsuite: test-fwrite fails with git glibc

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Tue, Nov 27, 2012 at 12:05:22PM -0500, Carlos O'Donell wrote:
> Returning 0 here violates POSIX which says "shall return the number of
> elements successfully written, which may be less than nitems if a
> write error is encountered."
> 
> What's your estimate on how hard is it to actually fix this correctly
> and return the number of bytes written?
> 

Actually I think my comment is still wrong and we're actually doing
the right thing now.  _IO_sputn will return EOF only if nothing was
written.  It returns partial success if it has managed to write
something into the buffer and a subsequent overflow failed:

...
      /* Next flush the (full) buffer. */
      if (_IO_OVERFLOW (f, EOF) == EOF)
        /* If nothing else has to be written or nothing has been written, we
           must not signal the caller that the call was even partially
           successful.  */
        return (to_do == 0 || to_do == n) ? EOF : n - to_do;
...

Here's the updated patch with the comment removed.

Regards,
Siddhesh

ChangeLog:

	* libio/Makefile (tests): Add test case tst-fwrite-error.
	* libio/iofwrite.c (_IO_fwrite): Return 0 on EOF.
	* libio/iofwrite_u.c (fwrite_unlocked): Likewise.
	* libio/tst-fwrite-error.c: New test case.

diff --git a/libio/Makefile b/libio/Makefile
index 9ccd6a0..83d90d0 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -59,7 +59,8 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-memstream1 tst-memstream2 \
 	tst-wmemstream1 tst-wmemstream2 \
 	bug-memstream1 bug-wmemstream1 \
-	tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos bug-fclose1 tst-fseek
+	tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos bug-fclose1 tst-fseek \
+	tst-fwrite-error
 ifeq (yes,$(build-shared))
 # Add test-fopenloc only if shared library is enabled since it depends on
 # shared localedata objects.
diff --git a/libio/iofwrite.c b/libio/iofwrite.c
index d4610f7..8571352 100644
--- a/libio/iofwrite.c
+++ b/libio/iofwrite.c
@@ -43,11 +43,11 @@ _IO_fwrite (buf, size, count, fp)
     written = _IO_sputn (fp, (const char *) buf, request);
   _IO_release_lock (fp);
   /* We have written all of the input in case the return value indicates
-     this or EOF is returned.  The latter is a special case where we
-     simply did not manage to flush the buffer.  But the data is in the
-     buffer and therefore written as far as fwrite is concerned.  */
-  if (written == request || written == EOF)
+     this.  */
+  if (written == request)
     return count;
+  else if (written == EOF)
+    return 0;
   else
     return written / size;
 }
diff --git a/libio/iofwrite_u.c b/libio/iofwrite_u.c
index a1077ee..e4a39fb 100644
--- a/libio/iofwrite_u.c
+++ b/libio/iofwrite_u.c
@@ -45,11 +45,11 @@ fwrite_unlocked (buf, size, count, fp)
     {
       written = _IO_sputn (fp, (const char *) buf, request);
       /* We have written all of the input in case the return value indicates
-	 this or EOF is returned.  The latter is a special case where we
-	 simply did not manage to flush the buffer.  But the data is in the
-	 buffer and therefore written as far as fwrite is concerned.  */
-      if (written == request || written == EOF)
+	 this.  */
+      if (written == request)
 	return count;
+      else if (written == EOF)
+	return 0;
     }
 
   return written / size;
diff --git a/libio/tst-fwrite-error.c b/libio/tst-fwrite-error.c
new file mode 100644
index 0000000..3c0cf49
--- /dev/null
+++ b/libio/tst-fwrite-error.c
@@ -0,0 +1,66 @@
+/* Test of fwrite() function, adapted from gnulib-tests in grep.
+   Copyright (C) 2011-2012 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 <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdio.h>
+
+static int
+do_test (void)
+{
+  char tmpl[] = "/tmp/tst-fwrite-error.XXXXXX";
+  int fd = mkstemp (tmpl);
+  if (fd == -1)
+    {
+      printf ("mkstemp failed with errno %d\n", errno);
+      return 1;
+    }
+  FILE *fp = fdopen (fd, "w");
+  if (fp == NULL)
+    {
+      printf ("fdopen failed with errno %d\n", errno);
+      return 1;
+    }
+
+  char buf[5] = "world";
+  setvbuf (fp, NULL, _IONBF, 0);
+  close (fd);
+  unlink (tmpl);
+  errno = 0;
+
+  int ret = fwrite (buf, 1, sizeof (buf), fp);
+  if (ret != 0)
+    {
+      printf ("fwrite returned %d\n", ret);
+      return 1;
+    }
+  if (errno != EBADF)
+    {
+      printf ("Errno is not EBADF: %d\n", errno);
+      return 1;
+    }
+  if (ferror (fp) == 0)
+    {
+      printf ("ferror not set\n");
+      return 1;
+    }
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"