This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: Ping: [PATCH] grep testsuite: test-fwrite fails with git glibc
On Tue, Nov 27, 2012 at 12:31 PM, Siddhesh Poyarekar
<siddhesh@redhat.com> wrote:
> 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.
Thanks for digging into this. I agree, I looked over
_IO_new_file_xsputn and it would seem that all one needs to do is
convert the EOF to zero in _IO_fwrite and fwrite_unlocked.
> 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. */
The new comment isn't very useful.
I'd suggest:
/* We are guaranteed to have written all of the input, none of it, or
some of it. */
(with appropriate wrapping).
> + 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. */
Same comment suggestion here.
> + 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"
>
OK with changes to the comment.
Cheers,
Carlos.