[PATCH] prevent retries on fclose/fflush after write errors

[Denys Vlasenko]

On Monday 18 February 2013 20:58, Bernhard Reutner-Fischer wrote:
> On 25 March 2012 07:54, Mike Frysinger <vapier at gentoo.org> wrote:
> > fixed the style & pushed.  thanks all!  it's nice we have people versed in
> > esoteric low level aspects nowadays.
> 
> hmz, revisiting..
> To recap:
> 
> On 7 February 2011 02:41, Denys Vlasenko <vda.linux at googlemail.com> wrote:
> > Currently, uclibc retains buffered data on stdio write errors,
> > and subsequent fclose and fflush will try to write it out again
> > (in most cases, in vain).
> >
> > Which results in something like this:
> >
> > On Wednesday 26 January 2011 13:21, Baruch Siach wrote:
> >> Hi busybox list,
> >>
> >> I'm running the following command under strace (thanks Rob):
> >>
> >> echo 56 > /sys/class/gpio/export
> >>
> >> and I see the following output:
> >>
> >> write(1, "56\n", 3)                     = -1 EBUSY (Device or resource busy)
> >> write(1, "5", 1)                        = 1
> >>
> >> The first EBUSY is OK, since GPIO 56 is already requested. But the second
> >> write() attempt seems strange, and leads to an unwanted outcome. GPIO 5 gets
> >> exported.
> >
> >
> > This patch prevents that.
> 
> This rather sounds to me that this second write with an odd length is wrong, no?

> IIUC the patch (867bac0c) that went in for this issue is causing
> https://bugs.uclibc.org/5156 (fclose and fflush do not report write failures)
> 
> I am attaching denys.c, i think this is the testcase you had in mind
> in your patch that Mike applied as
> 867bac0c750401d2f429ad6bb066498c3b8b35c1
> The attached fulltest.c is from above mentioned PR5156.
> Also attached is a patch that seems to cure PR5156 (the hunk against
> _wcommit.c is your rephrased patch cited below, not needed for 5156
> though) and that basically reverts your 867bac0c.
> With this patch we would get:
> $ ./denys_uc > /dev/null
> Hi there 1
> Hi there 2
> $ ./denys_uc 2> /dev/null
> $ ./denys_glibc > /dev/null
> Hi there 1
> Hi there 2
> $ ./denys_glibc 2> /dev/null
> $ ./fulltest_glibc
> fwrite: x=4, errno=25
> fflush: y=-1, errno=28
> fputc 1: x=88, errno=25
> fflush: y=-1, errno=28
> fputc 2: x=88, errno=25
> fclose: y=-1, errno=28
> # the ENOTYPEWRITER i dislike
> $ ./fulltest
> fwrite: x=4, errno=0
> fflush: y=-1, errno=28
> fputc 1: x=88, errno=28
> fflush: y=-1, errno=28
> fputc 2: x=88, errno=28
> fclose: y=-1, errno=28
> so 5156 would be fixed, we'd diverge from glibc in the type-writer
> errno (which is imo ok) and (since we fully buffer fwrite) do not
> errno the fwrite.

Testcases? Good! Please put them into test/* when you're done.


> I guess it would not fix your (line-buffered, i assume?) gpio echo, would it?

I don't know whether it will fix that. I don't have the testcase handy.

In my testing after this patch uclibc drops all output after error occurs.

I have a test program which sets stdout to nonblocking mode, writes lines until
printf fails, then resets stdouty to blocking mode, does fflush(NULL),
and prints last printf result and count of lines.

I run it like this:   ./a.out | { sleep 1; cat; }

Before your patch, the result was:
Hi there 000000001
Hi there 000000002
Hi there 000000003
...
Hi there 000002644
Hi there 000002645
Hi there 00000
n:-1
Bye 000002647


After this patch, output just ends:
...
Hi there 000002590
Hi there 000002591
Hi there 000002592 <---no newline char printed here

strace of the above:
...
18:39:29.777493 write(1, "\nHi there 000002539\nHi there 000002540\nHi there 000002541\nHi there 000002542\nHi there 000002543\nHi "..., 1022) = 1022
18:39:29.777550 write(1, "2592", 4)     = 4
18:39:29.777640 write(1, "\nHi there 000002593\nHi there 000002594\nHi there 000002595\nHi there 000002596\nHi there 000002597\nHi "..., 1022) = -1 EAGAIN (Resource temporarily u
navailable)
18:39:29.777702 fcntl(1, F_GETFL)       = 0x801 (flags O_WRONLY|O_NONBLOCK)
18:39:29.777759 fcntl(1, F_SETFL, O_WRONLY) = 0
18:39:29.777814 _exit(0)                = ?
18:39:29.777870 +++ exited with 0 +++

IOW: EAGAIN become a fatal, data-gobbling error,
stream will remain inoperative even after error goes away (!).
The final printf in my program (see source) just didn't do anything.

glibc does this:
...
Hi there 000003449
Hi th
n:-1
Bye 000003666

As you see, glibc doesn't "kill" the stream on EAGAIN.



I think this part of the patch is wrong:

-               } else {
+               } else /* if (errno != EINTR && errno != EAGAIN) */ {
.
                        __STDIO_STREAM_SET_ERROR(stream);
-
+#if 0
                        /* We buffer data on "transient" errors, but discard it
                         * on "hard" ones. Example of a hard error:
                         *
@@ -81,6 +81,7 @@ size_t attribute_hidden __stdio_WRITE(register FILE *stream,
                                /* do we have other "soft" errors? */
                                break;
                        }
+#endif
 #ifdef __STDIO_BUFFERS
                        stodo = __STDIO_STREAM_


it simple drops special handling of EAGAIN.


The test program follows.
-- 
vda

#include <unistd.h>
#include <stdio.h>
#include <errno.h>
#include <assert.h>
#include <fcntl.h>
static void ndelay_on(int fd)
{
        int flags = fcntl(fd, F_GETFL);
        if (flags & O_NONBLOCK)
                return;
        fcntl(fd, F_SETFL, flags | O_NONBLOCK);
}
static void ndelay_off(int fd)
{
        int flags = fcntl(fd, F_GETFL);
        if (!(flags & O_NONBLOCK))
                return;
        fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
}
int main(void) {
        int i, n;
        int newfd = fileno(stdout);

        ndelay_on(newfd);
        i = 1;
        while (1) {
                n = printf("Hi there %09d\n", i++);
                if (n != 19)
                        break;
        }
        ndelay_off(newfd);
        fflush(NULL);
        printf("\nn:%d\nBye %09d\n", n, i++);
        return 0;
}