Re: [PATCH] prevent retries on fclose/fflush after write errors
On Monday 18 February 2013 20:58, Bernhard Reutner-Fischer wrote:
On 25 March 2012 07:54, Mike Frysinger vap...@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.li...@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 1
Hi there 2
Hi there 3
...
Hi there 02644
Hi there 02645
Hi there 0
n:-1
Bye 02647
After this patch, output just ends:
...
Hi there 02590
Hi there 02591
Hi there 02592 ---no newline char printed here
strace of the above:
...
18:39:29.777493 write(1, \nHi there 02539\nHi there 02540\nHi there
02541\nHi there 02542\nHi there 02543\nHi ..., 1022) = 1022
18:39:29.777550 write(1, 2592, 4) = 4
18:39:29.777640 write(1, \nHi there 02593\nHi there 02594\nHi there
02595\nHi there 02596\nHi there 02597\nHi ..., 1022) = -1 EAGAIN
(Resource temporarily u
navailable)
18:39:29.02 fcntl(1, F_GETFL) = 0x801 (flags O_WRONLY|O_NONBLOCK)
18:39:29.59 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 03449
Hi th
n:-1
Bye 03666
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
Re: [PATCH] prevent retries on fclose/fflush after write errors
On 25 March 2012 07:54, Mike Frysinger vap...@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.li...@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.
I guess it would not fix your (line-buffered, i assume?) gpio echo, would it?
--
vda
diff -d -urpN uClibc.0/libc/stdio/_wcommit.c uClibc.1/libc/stdio/_wcommit.c
--- uClibc.0/libc/stdio/_wcommit.c 2011-02-07 00:04:34.0 +0100
+++ uClibc.1/libc/stdio/_wcommit.c 2011-02-07 00:55:24.0 +0100
@@ -20,7 +20,18 @@ size_t attribute_hidden __stdio_wcommit(
__STDIO_STREAM_VALIDATE(stream);
- if ((bufsize = __STDIO_STREAM_BUFFER_WUSED(stream)) != 0) {
+ /* Note: we do not write anything if write error has been detected.
+* Otherwise, stdio user has no way to prevent retries after
+* failed write - and some users do want to not have any retries!
+* IOW: if write errored out, neither fflush nor fclose should
+* try to write buffered data.
+* clearerr may be used to enable retry if needed.
+*/
+
+ bufsize = __STDIO_STREAM_BUFFER_WUSED(stream);
+ if (bufsize != 0
+ !(stream-__modeflags __FLAG_ERROR)
+ ) {
stream-__bufpos = stream-__bufstart;
__stdio_WRITE(stream, stream-__bufstart, bufsize);
}
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
attaching..
On 18 February 2013 20:58, Bernhard Reutner-Fischer
rep.dot@gmail.com wrote:
On 25 March 2012 07:54, Mike Frysinger vap...@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.li...@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.
I guess it would not fix your (line-buffered, i assume?) gpio echo, would it?
--
vda
diff -d -urpN uClibc.0/libc/stdio/_wcommit.c uClibc.1/libc/stdio/_wcommit.c
--- uClibc.0/libc/stdio/_wcommit.c 2011-02-07 00:04:34.0 +0100
+++ uClibc.1/libc/stdio/_wcommit.c 2011-02-07 00:55:24.0 +0100
@@ -20,7 +20,18 @@ size_t attribute_hidden __stdio_wcommit(
__STDIO_STREAM_VALIDATE(stream);
- if ((bufsize = __STDIO_STREAM_BUFFER_WUSED(stream)) != 0) {
+ /* Note: we do not write anything if write error has been detected.
+* Otherwise, stdio user has no way to prevent retries after
+* failed write - and some users do want to not have any retries!
+* IOW: if write errored out, neither fflush nor fclose should
+* try to write buffered data.
+* clearerr may be used to enable retry if needed.
+*/
+
+ bufsize = __STDIO_STREAM_BUFFER_WUSED(stream);
+ if (bufsize != 0
+ !(stream-__modeflags __FLAG_ERROR)
+ ) {
stream-__bufpos = stream-__bufstart;
__stdio_WRITE(stream, stream-__bufstart, bufsize);
}
#include unistd.h
#include stdio.h
#include errno.h
#include assert.h
int main(void) {
int i;
int oldfd = fileno(stderr);
int newfd = fileno(stdout);
i = close(newfd);
assert(i == 0);
i = printf(Hi there 1\n);
assert(i == 11);
i = ferror(stdout);
assert(i == 0);
i = dup2(oldfd, newfd);
assert(i == newfd);
i = printf(Hi there 2\n);
assert(i == 11);
return 0;
}
#include assert.h
#include stdio.h
#include stdlib.h
#include string.h
#include errno.h
int
main (int argc, char **argv)
{
FILE *f;
int x, y;
char str[1024];
memset (str, 'X', sizeof str);
/* subcase 0 */
f = fopen (/dev/full, w);
assert (f);
x = fwrite (str, 1024/4, 4, f);
printf(fwrite: x=%d, errno=%d\n, x, errno);
y = fflush (f);
printf(fflush: y=%d, errno=%d\n, y, errno);
assert (x == EOF || y == EOF);
fclose (f);
/* subcase 1 */
f = fopen (/dev/full, w);
assert (f);
x = fputc ('X', f);
printf(fputc 1: x=%d, errno=%d\n, x, errno);
y = fflush (f);
printf(fflush: y=%d, errno=%d\n, y, errno);
assert (x == EOF || y == EOF);
fclose (f);
/* subcase 2 */
f = fopen (/dev/full, w);
assert (f);
x = fputc ('X', f);
printf(fputc 2: x=%d, errno=%d\n, x, errno);
y = fclose (f);
printf(fclose: y=%d, errno=%d\n, y, errno);
assert (x == EOF || y == EOF);
return EXIT_SUCCESS;
}
uClibc-redo-867bac0c750401d2f429ad6bb066498c3b8b35c1.00.patch
Description:
Re: [PATCH] prevent retries on fclose/fflush after write errors
fixed the style pushed. thanks all! it's nice we have people versed in esoteric low level aspects nowadays. -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Wed, 14 Mar 2012, Rich Felker wrote: It was cited in Worse is Better, yes, but I don't entirely believe the anecdote. SA_RESTART semantics are no harder to implement; it's just a matter of whether you save the address of the syscall instruction or the instruction immediately after it when invoking a signal handler. Usually the address instruction immediately after will already be on the stack, or some sort of task structure, as part of the normal syscall process that would be followed in the absence of signals. Now, adjusting the saved instruction pointer would not be hard in machine code. Generally, the saved user IP is sitting somewhere in the kernel stack a predictable distance above the return address for the kernel function implementing the call, like a hidden extra argument. But access to such context isn't something C compilers are good at, and the stack offset and syscall opcode length must be customized for each processor family. The ancient worse-is-better Unix programmers probably decided to just foreclose on syscalls doing anything to the userspace registers (aside from the one used to return results). This made the assembly glue joining a CPU-agnostic C kernel to a specific machine as simple as possible, and made EINTR the only acceptable way. It can be solved (albeit in an ugly way) by having the signal handler re-arm the alarm with exponential falloff in delay (in case the system is so loaded that it can't return from the signal handler before another timer expiration happens). Yuck. That's a lousy quality of implemantion, and isn't even adequate for probing whether a system implements EINTR. Even if your approach is preferable to users, I don't think it's conformant, since POSIX specifies the EINTR error for fgetc. Since all That may be just tolerance for a lazy stdio that doesn't work any harder than it needs to. Certainly no one disputes a stdio is *allowed* to treat EINTR same as EIO. The question is whether it *must*. Michael Deutschmann mich...@talamasca.ocis.net ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 10:58:33AM -0400, Rich Felker wrote: On Tue, Mar 13, 2012 at 10:30:10AM +0100, Laurent Bercot wrote: Why not simply let the user set SA_RESTART in the sigaction() call? see http://www.skarnet.org/software/skalibs/libstddjb/safewrappers.html I know of no modern system where this kind of bogus EINTR happens (SIGSTOP, etc.). Even the proprietary unices fixed it before Linux did, but they've all been fixed for a long time now. I agree the standard should be more explicit on this; failure to be explicit, if you interpret it literally, makes interrupting signals unusable since you have to wrap all your syscalls with loops and make them behave as if the signal was a restarting one.. FWIW, Michael Kerrisk's book The Linux Programming Interface lists a number of syscalls which can return EINTR after resume from SIGSTOP. Normal read() is not one of them, but e.g. read() from inotify fd is. I would also expect that some character devices do not implement restarting for read() and write(), if just because ERESTARTSYS etc. are not sufficiently documented. Also, the system calls for which SA_RESTART is effective seems to change over time, e.g. before 2.6.22 futex(), sem_wait and sem_timedwait() did not observe SA_RESTART. (There's about a page listing various syscall's behaviour with SA_RESTART in TLPI book.) Johannes ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, 13 Mar 2012, Rich Felker wrote: I would consider this a flaw in the standard since it largely prevents using EINTR in any useful way. EINTR wasn't invented to be useful, it was invented because it was easier to implement in pre-sigaction() SysV kernels than SA_RESTART semantics. Known as the PCLSR problem, it is an often cited example of the Worse is Better design philosophy at work. I was assuming the old standard idiom of installing a do-nothing signal handler (without SA_RESTART) for SIGALRM so that fgets would return with an error and the program could proceed. That's a broken idiom, since if the signal should arrive just one opcode before the read syscall begins, it will be ignored. If you need reliable signal interruption, you must use sigsuspend() or longjmp out of the handler. It's also the only possible use of a cleared SA_RESTART, so it's crazy not to set the flag all the time. That we have a SA_RESTART flag at all, is just a measure so that old SysV programs abusing the design flaw break occasionally instead of often. One historical note: Circa 2000, I was actively reading glibc's bugs list and trying to help out. Someone posted a bug report (libc/1174 in the old GNATS system) citing this very issue, and I suggested a patch to restart after EINTR within stdio -- since any deliberate use of interruption would involve a race condition. Ulrich Drepper denied it, on the grounds that: ) But this is not correct. You must be able to set an alarm() and ) terminate a blocked fwrite() code. This is required and tested by all ) kinds of standard test suites. Although that exchange eventually had one productive result -- the node Error Recovery in the glibc manual, which I basically wrote. Sounds like in the intervening decade, glibc may have come around to my thinking Michael Deutschmann mich...@talamasca.ocis.net ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Monday 12 March 2012 01:27:08 Kevin Cernekee wrote:
+ if (errno != EINTR
+ errno != EAGAIN
+ /* do we have other soft errors? */
+ ) {
+ break;
stylewise, this break is missing a tab too :p
otherwise, this looks good to go, no one has any complaints with it, so should
be good to merge ...
-mike
signature.asc
Description: This is a digitally signed message part.
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Wed, Mar 14, 2012 at 02:26:06PM +0100, Johannes Stezenbach wrote: On Tue, Mar 13, 2012 at 10:58:33AM -0400, Rich Felker wrote: On Tue, Mar 13, 2012 at 10:30:10AM +0100, Laurent Bercot wrote: Why not simply let the user set SA_RESTART in the sigaction() call? see http://www.skarnet.org/software/skalibs/libstddjb/safewrappers.html I know of no modern system where this kind of bogus EINTR happens (SIGSTOP, etc.). Even the proprietary unices fixed it before Linux did, but they've all been fixed for a long time now. I agree the standard should be more explicit on this; failure to be explicit, if you interpret it literally, makes interrupting signals unusable since you have to wrap all your syscalls with loops and make them behave as if the signal was a restarting one.. FWIW, Michael Kerrisk's book The Linux Programming Interface lists a number of syscalls which can return EINTR after resume from SIGSTOP. It's outdated. See man 7 signal for details on what has/hasn't been fixed. The only ones still broken are all in the timed wait family, for which isn't not that big a deal to return early anyway since the caller expects that a timed wait operation might fail with a timeout and already has to deal with this error case. Normal read() is not one of them, but e.g. read() from inotify fd is. While this is broken in principle, inotify is enough of an oddball (and only really usable paired with select/poll anyway) that it probably doesn't matter. I would also expect that some character devices do not implement restarting for read() and write(), if just because ERESTARTSYS etc. are not sufficiently documented. I'm not aware of any such issues, at least not with the devices normal user apps would interface with. The main type, terminals, are not buggy in this regard (but are buggy in regard to readv/writev in a different manner). It's actually really frustrating that Linux exposes the potential brokenness of each underlying driver to the application. The read syscall itself should be enforcing POSIX conformance regardless of how the underlying driver behaves. Also, the system calls for which SA_RESTART is effective seems to change over time, e.g. before 2.6.22 futex(), sem_wait and sem_timedwait() did not observe SA_RESTART. (There's about a page listing various syscall's behaviour with SA_RESTART in TLPI book.) Yes, that was a serious bug. Thankfully it's been fixed, and there are enough other relatively important fixes around the same time that it's a really bad idea to be using such an old kernel for applications that use threads. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 07:16:54AM +, u-uclibc-q...@aetey.se wrote: On Tue, Mar 13, 2012 at 01:58:49AM -0400, Rich Felker wrote: On Tue, Mar 13, 2012 at 01:41:01AM -0400, Mike Frysinger wrote: int __filedes; + int __errno_value; pretty sure this breaks ABI. i know we aren't completely strict on this, but it's something we should avoid if possible. How so? Application code should not be touching the internals of FILE... You are right Rich but it seems you are thinking of API while this change breaks ABI. If it does, I still don't see how it could. Applications cannot declare objects of type FILE, only FILE *. Short of some macros that poke at the early members of the structure (getc/putc macros), it's purely an opaque type, so changes to its size or layout (beyond said early members) should not break ABI. If you claim it does, please explain how you think it does, because the rest of us are missing that issue.. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 10:30:10AM +0100, Laurent Bercot wrote: Note that I'm claiming the whole safe_read/safe_write idiom is wrong, and a throwback to the pre-sigaction SysV era when signal() was the only way to install a signal handler and it gave you non-restarting semantics. The idea of an interrupting signal, when set intentionally by a modern application using sigaction, is that EINTR should be treated as a (usually permanent/unrecoverable) failure on blocked in-progress IO operations. Hi Rich, I'm not pronouncing on stdio, since stdio doesn't really mix with asynchronous event loops anyway; but in an event-driven program that reacts to signals as well as fd events or timeouts, signals can arrive at any time and should be handled by the application at the same level as any other event, in a normal context, *not* right when they arrive in an interrupt context. (Think pselect(), or the self-pipe trick.) I agree that interrupting signals are not very useful for an event-driven application. This is not their intended usage case. In this case, it is absolutely necessary to protect every read() and write() operation, as well as any interruptible system call, against EINTR, because EINTR is not reporting a failure, it is only reporting the arrival of a signal at an inconvenient time. EINTR is basically reporting that the user hit Ctrl+C and the application wants the library routine that was in progress to return with an error rather than asynchronously exiting, probably because it has unwritten state that can't be accessed asynchronously for saving. There is no reason to protect reads against EINTR because EINTR cannot happen unless the application specifically chooses for it to happen (by installing a non-restarting signal handler). EINTR does not just randomly happen. This protection can be done at the user level, but there is nothing wrong My claim is that code doing such protection is wrong and harmful to begin with. Why not simply let the user set SA_RESTART in the sigaction() call? see http://www.skarnet.org/software/skalibs/libstddjb/safewrappers.html I know of no modern system where this kind of bogus EINTR happens (SIGSTOP, etc.). Even the proprietary unices fixed it before Linux did, but they've all been fixed for a long time now. I agree the standard should be more explicit on this; failure to be explicit, if you interpret it literally, makes interrupting signals unusable since you have to wrap all your syscalls with loops and make them behave as if the signal was a restarting one.. I think the difference in opinion about EINTR comes from two different ideological camps: one which believes the Worse is Better story that the very existence of EINTR was a bug in early Unix systems, and another which believes EINTR was/is a feature to allow a program interrupted during a blocked operation by a (usually user-generated) signal to terminate or return to an initial state cleanly without breaking the rules of async signal safety, discarding whatever intermediate work it was doing. Think of programs that do something like: alarm(1); fgets(buf, sizeof buf, f); Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 01:46:39AM -0400, Mike Frysinger wrote: treating EINTR/EAGAIN as unrecoverable is bad behavior for an app. there are valid ways an app could receive either signal and have it be normal behavior. Receiving a signal does not result in EINTR. Running a signal handler that was installed (intentionally) without SA_RESTART is what causes EINTR. having that handling be done in uClibc is not required by POSIX, but it's better imo for uClibc to carry a little bit more code there to provide better reliability to higher layers rather than forcing every userspace app to implement their own retry-on-eagain macros. Retrying on EAGAIN is simply *wrong*. It makes you consume 100% cpu. If the file descriptor is set as EAGAIN, it means whoever set it up wants to ensure that blocking does not happen; emulating blocking with 100% cpu load is the worst possible way to handle this situation! For stdio, EAGAIN must be treated as a hard error. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
Hi Rich, On Tue, Mar 13, 2012 at 10:36:48AM -0400, Rich Felker wrote: On Tue, Mar 13, 2012 at 01:41:01AM -0400, Mike Frysinger wrote: int __filedes; + int __errno_value; You are right Rich but it seems you are thinking of API while this change breaks ABI. If it does, I still don't see how it could. Applications cannot declare objects of type FILE, only FILE *. Short of some macros that poke at the early members of the structure (getc/putc macros), it's Applications actually can declare, allocate, copy FILE objects unless explicitely prohibited by the specification - does it prohibit this? All I found is a mention that a copy of a FILE object is unusable at a different address than the original (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf page 266 last paragraph). I can not think of an apparent meaningful reason for copying FILE or using sizeof(FILE) but this by itself doesn't mean there isn't any. (sorry for a phrase with three negations...) That's why I assume that the FILE structure as a whole is a part of the ABI, despite being opaque. I may be wrong reading/interpreting specification(s), then please correct me. The actual change being discussed may be compatible in practice, I take your word for this! Rich Regards, Rune ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 04:06:35PM +, u-uclibc-q...@aetey.se wrote: Hi Rich, On Tue, Mar 13, 2012 at 10:36:48AM -0400, Rich Felker wrote: On Tue, Mar 13, 2012 at 01:41:01AM -0400, Mike Frysinger wrote: int __filedes; + int __errno_value; You are right Rich but it seems you are thinking of API while this change breaks ABI. If it does, I still don't see how it could. Applications cannot declare objects of type FILE, only FILE *. Short of some macros that poke at the early members of the structure (getc/putc macros), it's Applications actually can declare, allocate, copy FILE objects unless explicitely prohibited by the specification - does it prohibit this? All I found is a mention that a copy of a FILE object is unusable at a different address than the original (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf page 266 last paragraph). Applications presumably can do this, but the only use for doing so would be to use the object as a buffer of type `char[sizeof(FILE)]`. It can't be used for any purpose connected with stdio. And being that the size is very arbitrary, I can't see how you could use it for anything useful; the only thing you know about `sizeof(FILE)` is that it's greater than or equal to 1. Presumably it could even have size SIZE_MAX to prevent applications from declaring objects of type FILE... :-) That's why I assume that the FILE structure as a whole is a part of the ABI, despite being opaque. I may be wrong reading/interpreting specification(s), then please correct me. The actual change being discussed may be compatible in practice, I take your word for this! I believe you may be technically correct, but there's no correct/sane program that would be affected by this ABI change. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
There is no reason to protect reads against EINTR because EINTR cannot happen unless the application specifically chooses for it to happen (by installing a non-restarting signal handler). EINTR does not just randomly happen. It can, if you take the standard literally. I agree that you *should* be able to assume that unblockable signals have SA_RESTART behaviour, that would simplify Unix programming a whole lot. But as of today, you can't, and I'd rather be on the safe side. My claim is that code doing such protection is wrong and harmful to begin with. I disagree, because it has its uses. But I think we can agree that it is wrong to do it *in the libc*. The programmer should have the choice to handle EINTR himself. Think of programs that do something like: alarm(1); fgets(buf, sizeof buf, f); SIGALRM is raised. SIGALRM isn't caught. SIGALRM kills the process, even if it was blocking on a safe_read() function. Where's the problem? -- Laurent ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 06:59:03PM +0100, Laurent Bercot wrote: There is no reason to protect reads against EINTR because EINTR cannot happen unless the application specifically chooses for it to happen (by installing a non-restarting signal handler). EINTR does not just randomly happen. It can, if you take the standard literally. I agree that you *should* be able to assume that unblockable signals have SA_RESTART behaviour, that would simplify Unix programming a whole lot. But as of today, you can't, and I'd rather be on the safe side. I'd have to go back and look at the (disorganized) language about it in the standard, but you may be right about the lack of a strict requirement. I would consider this a flaw in the standard since it largely prevents using EINTR in any useful way. There's a similar bug in the statement of pthread cancellation behavior that makes it so implementations are allowed (and glibc/uClibc exercise this allowance!) to leak resources or worse (corrupt fd state) randomly when cancellation is performed; allowing implementations of such low quality essentially makes the feature unusable. This should really all be documented well and submitted as defect reports... My claim is that code doing such protection is wrong and harmful to begin with. I disagree, because it has its uses. But I think we can agree that it is wrong to do it *in the libc*. The programmer should have the choice to handle EINTR himself. In some places in a program you might know it's essential to complete the operation and not lose data, in which case you're right, but this sort of situation will require specific contracts between the callee and caller about what to do. In general, I feel like the appropriate response to EINTR in the absence of a contract with the caller is to assume that when the caller has arranged for EINTR to happen, the caller wants you to stop when interrupted. Think of programs that do something like: alarm(1); fgets(buf, sizeof buf, f); SIGALRM is raised. SIGALRM isn't caught. SIGALRM kills the process, even if it was blocking on a safe_read() function. Where's the problem? I was assuming the old standard idiom of installing a do-nothing signal handler (without SA_RESTART) for SIGALRM so that fgets would return with an error and the program could proceed. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 01:14:12PM -0400, Rich Felker wrote: SIZE_MAX to prevent applications from declaring objects of type FILE... :-) :-) I guess you are right and nobody declares them. there's no correct/sane program that would be affected by this ABI change. Actually I suspect that stdio macros might use stuff (like __bufpos) which is located past the proposed new structure member, in which case existing binaries would become incompatible with a patched library: #endif /* __UCLIBC_HAS_WCHAR__ */ int __filedes; + int __errno_value; #ifdef __STDIO_BUFFERS unsigned char *__bufstart; /* pointer to buffer */ unsigned char *__bufend;/* pointer to 1 past end of buffer */ - unsigned char *__bufpos; Rune ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 07:19:35PM +, u-uclibc-q...@aetey.se wrote: there's no correct/sane program that would be affected by this ABI change. Actually I suspect that stdio macros might use stuff (like __bufpos) which is located past the proposed new structure member, in which case existing binaries would become incompatible with a patched library: #endif /* __UCLIBC_HAS_WCHAR__ */ int __filedes; + int __errno_value; #ifdef __STDIO_BUFFERS unsigned char *__bufstart; /* pointer to buffer */ unsigned char *__bufend;/* pointer to 1 past end of buffer */ - unsigned char *__bufpos; Indeed, I missed this. It should definitely not be added near the beginning of the structure, only past elements that could be part of the macro ABI. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tuesday 13 March 2012 01:58:49 Rich Felker wrote:
On Tue, Mar 13, 2012 at 01:41:01AM -0400, Mike Frysinger wrote:
On Sunday 11 March 2012 11:12:19 Denys Vlasenko wrote:
--- a/libc/sysdeps/linux/common/bits/uClibc_stdio.h
+++ b/libc/sysdeps/linux/common/bits/uClibc_stdio.h
@@ -250,6 +250,7 @@ struct __STDIO_FILE_STRUCT {
unsigned char __ungot[2];
#endif /* __UCLIBC_HAS_WCHAR__ */
int __filedes;
+ int __errno_value;
#ifdef __STDIO_BUFFERS
unsigned char *__bufstart; /* pointer to buffer */
unsigned char *__bufend;/* pointer to 1 past end of buffer */
pretty sure this breaks ABI. i know we aren't completely strict on this,
but it's something we should avoid if possible.
How so? Application code should not be touching the internals of
FILE...
because the uClibc macros which implement the public API directly access the
structure contents. look at the getc macros and how it leverages the locking
members.
-mike
signature.asc
Description: This is a digitally signed message part.
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tuesday 13 March 2012 15:32:01 Rich Felker wrote: On Tue, Mar 13, 2012 at 07:19:35PM +, u-uclibc-q...@aetey.se wrote: there's no correct/sane program that would be affected by this ABI change. Actually I suspect that stdio macros might use stuff (like __bufpos) which is located past the proposed new structure member, in which case existing binaries would become incompatible with a patched library: #endif /* __UCLIBC_HAS_WCHAR__ */ int __filedes; + int __errno_value; #ifdef __STDIO_BUFFERS unsigned char *__bufstart; /* pointer to buffer */ unsigned char *__bufend;/* pointer to 1 past end of buffer */ - unsigned char *__bufpos; Indeed, I missed this. It should definitely not be added near the beginning of the structure, only past elements that could be part of the macro ABI. that would help only if the uClibc code itself had versioned functions which handled the growing struct. if you're attempting to support code that allocated sizeof(FILE) memory (which is dumb), then location in the struct wouldn't matter. -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 04:24:33PM -0400, Mike Frysinger wrote: It should definitely not be added near the beginning of the structure, only past elements that could be part of the macro ABI. that would help only if the uClibc code itself had versioned functions which handled the growing struct. Curious, why wouldn't this work without versioned functions? FILE structures are expected to be allocated by the library and used by 1. the library and 2. the macros. If we just add extra items at the end, neither the new library code would be confused (it knows about the new size and contents), nor the old application code would be confused (the macros use[d] the offsets which did not change, the calls to the functions pass pointers not structures). Of course this generally only works if the macros are not being changed. if you're attempting to support code that allocated sizeof(FILE) memory (which is dumb), then location in the struct wouldn't matter. I don't think keeping the size of FILE constant is practically important, even though I mentioned this as an incompatibility. Anyway, everyone now seems to agree that the proposed patch breaks the ABI :) -mike Regards, Rune ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tuesday 13 March 2012 16:56:13 u-uclibc-q...@aetey.se wrote: On Tue, Mar 13, 2012 at 04:24:33PM -0400, Mike Frysinger wrote: It should definitely not be added near the beginning of the structure, only past elements that could be part of the macro ABI. that would help only if the uClibc code itself had versioned functions which handled the growing struct. Curious, why wouldn't this work without versioned functions? if end user code encodes sizeof(FILE) anywhere, they're creating a memory region of a fixed number of bytes, so increasing the structure means that the app will give us a pointer to memory of X bytes, but the library requires it to be Y (where Y is greater than X) bytes, so the library will clobber memory the user did not intend. declare char buf[sizeof(FILE)]; FILE *x = buf; on the stack and uClibc will overwrite random stuff on the stack. FILE structures are expected to be allocated by the library your sub-thread here indicated that someone other than uClibc was allocating FILE structures (or i misread). that cannot work w/out symbol versioning. if uClibc is the only thing that allocates/uses these structures, then appending members should be fine. Of course this generally only works if the macros are not being changed. correct, any existing members which are exposed via macros as part of the public API would have to remain in place. if you're attempting to support code that allocated sizeof(FILE) memory (which is dumb), then location in the struct wouldn't matter. I don't think keeping the size of FILE constant is practically important, even though I mentioned this as an incompatibility. if by practically you mean we don't care about the end users who are relying on sizeof(FILE), then sure (i don't care about that use case). i was just being pedantic in my reply. -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
alarm(1); fgets(buf, sizeof buf, f); I was assuming the old standard idiom of installing a do-nothing signal handler (without SA_RESTART) for SIGALRM so that fgets would return with an error and the program could proceed. Ugh. I get it. In this case, then, *of course* you want to get EINTR reported to the user. And I already agreed that the libc should not safe-wrap system calls, so yes, naturally fgets should be interrupted. But honestly, this seems to me like the lazy (in a bad sense) way to write such a program; the right way would be to write a little asynchronous select/poll loop with a timeout of 1 second and a reading event on fd 0. The main reason why the latter paradigm isn't used as much is that stdio doesn't handle partial reads and so is quite unusable in asynchronous loops - in other words, stdio sucks. Whenever I need to listen to signals as events (or, in the case of alarm(), as timeouts), I use a self-pipe and a select loop so I can listen to any number of events at the same time. It's a bit harder to program, but much more robust; the blocking system calls + EINTR paradigm is ad-hoc and error-prone. It can break so easily, for instance when your libc is misguided and safewraps around EINTR. ;) -- Laurent ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 04:24:33PM -0400, Mike Frysinger wrote: #endif /* __UCLIBC_HAS_WCHAR__ */ int __filedes; + int __errno_value; #ifdef __STDIO_BUFFERS unsigned char *__bufstart; /* pointer to buffer */ unsigned char *__bufend;/* pointer to 1 past end of buffer */ - unsigned char *__bufpos; Indeed, I missed this. It should definitely not be added near the beginning of the structure, only past elements that could be part of the macro ABI. that would help only if the uClibc code itself had versioned functions which handled the growing struct. if you're attempting to support code that No, there is no need for versioned functions. The uClibc code is in uClibc and will always be using the version of FILE corresponding to itself. Code in other libraries or applications only cares that FILE * is a pointer (it might as well be void *) and about the offsets/types of the buffer pointers (if getc/putc macros are enabled). allocated sizeof(FILE) memory (which is dumb), then location in the struct wouldn't matter. There's nothing useful/meaningful that such code could do. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 05:07:32PM -0400, Mike Frysinger wrote: On Tuesday 13 March 2012 16:56:13 u-uclibc-q...@aetey.se wrote: On Tue, Mar 13, 2012 at 04:24:33PM -0400, Mike Frysinger wrote: It should definitely not be added near the beginning of the structure, only past elements that could be part of the macro ABI. that would help only if the uClibc code itself had versioned functions which handled the growing struct. Curious, why wouldn't this work without versioned functions? if end user code encodes sizeof(FILE) anywhere, they're creating a memory region of a fixed number of bytes, so increasing the structure means that the app will give us a pointer to memory of X bytes, but the library requires it to be Y (where Y is greater than X) bytes, so the library will clobber memory the user did not intend. declare char buf[sizeof(FILE)]; FILE *x = buf; on the stack and uClibc will overwrite random stuff on the stack. Passing this FILE * to stdio functions invokes undefined behavior. They all require a FILE * obtained from fopen/fdopen/popen/etc. or from one of the standard stream pointers (stdin/stdout/stderr). The C standard is very explicit that you are not allowed to use copies of FILEs, and without copying an existing one, there's no way to construct one that would be valid anyway. FILE structures are expected to be allocated by the library your sub-thread here indicated that someone other than uClibc was allocating FILE structures (or i misread). that cannot work w/out symbol versioning. if uClibc is the only thing that allocates/uses these structures, then appending members should be fine. No one but uClibc can allocate them. Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Tue, Mar 13, 2012 at 11:15:32PM +0100, Laurent Bercot wrote: alarm(1); fgets(buf, sizeof buf, f); I was assuming the old standard idiom of installing a do-nothing signal handler (without SA_RESTART) for SIGALRM so that fgets would return with an error and the program could proceed. Ugh. I get it. In this case, then, *of course* you want to get EINTR reported to the user. And I already agreed that the libc should not safe-wrap system calls, so yes, naturally fgets should be interrupted. Yes, this is the sort of usage case I was talking about all along, and as far as I can tell, really the only reason anybody would ever want EINTR in the first place. Otherwise it's just a nuisance. But honestly, this seems to me like the lazy (in a bad sense) way to write such a program; the right way would be to write a little asynchronous select/poll loop with a timeout of 1 second and a reading event on fd 0. The main reason why the latter paradigm isn't used as much is that stdio doesn't handle partial reads and so is quite unusable in asynchronous loops - in other words, stdio sucks. There are at least 3 ways to do it. You, and many people these days, favor event-driven programming with select/poll/libevent/whatever. This is certainly one reasonable design, but it tends to make things that would otherwise be simple rather complicated, and precludes using stdio. I wouldn't say this means stdio sucks; rather, it's just meant for other types of programming. The other 2 ways are the one I described (using EINTR) and threads with thread cancellation as the method for getting out of blocked situations. Both of these work quite well with stdio. Whenever I need to listen to signals as events (or, in the case of alarm(), as timeouts), I use a self-pipe and a select loop so I can listen to any number of events at the same time. It's a bit harder to program, but much more robust; the blocking system calls + EINTR paradigm is ad-hoc and error-prone. It can break so easily, for instance when your libc is misguided and safewraps around EINTR. ;) Well I would call this not just misguided but wrong since POSIX specifies that fgetc/fputc (and all other stdio functions defined in terms of them) return failure with errno==EINTR for interrupting signals... Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Saturday 10 March 2012 23:55, Kevin Cernekee wrote:
On Sat, Feb 12, 2011 at 6:31 PM, Denys Vlasenko
vda.li...@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).
This seems to be causing some strange problems on bash as well. Is
there any interest in trying to mimic the glibc behavior?
uClibc version is 0.9.32.1.
bash problem:
# bash --version
GNU bash, version 3.2.0(1)-release (mipsel-unknown-linux-gnu)
Copyright (C) 2005 Free Software Foundation, Inc.
# echo none of the above /sys/power/state
sh: echo: write error: Invalid argument
# a
sh: a: command not found
none of the abov# b
sh: b: command not found
none of the abov#
# echo mem /sys/power/state
sh: echo: write error: Invalid argument
# echo mem /sys/power/state
sh: echo: write error: Invalid argument
# busybox echo mem /sys/power/state
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.01 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
Suspending console(s) (use no_console_suspend to debug)
Simple test case to show that the buffered data persists (albeit in
truncated form) even after calling fflush() and clearerr():
-- 8 --
#include stdio.h
#include unistd.h
#include fcntl.h
int main(int argc, char **argv)
{
int tmpfd, fd;
tmpfd = fcntl(fileno(stdout), F_DUPFD, 0);
fd = open(/dev/null, O_RDONLY);
dup2(fd, fileno(stdout));
printf(hello world\n);
fflush(stdout);
if (ferror(stdout))
clearerr(stdout);
dup2(tmpfd, fileno(stdout));
printf(goodbye world\n);
return 0;
}
-- 8 --
Output:
# ./test
hello worlgoodbye world
I propose the following patch.
Only compile tested by me, care to run-test?
--
vda
diff --git a/libc/stdio/_stdio.h b/libc/stdio/_stdio.h
index 2c0efc9..78069a5 100644
--- a/libc/stdio/_stdio.h
+++ b/libc/stdio/_stdio.h
@@ -212,7 +212,7 @@ extern int __stdio_seek(FILE *stream, register __offmax_t
*pos, int whence) attr
#define __STDIO_STREAM_SET_EOF(S) \
((void)((S)-__modeflags |= __FLAG_EOF))
#define __STDIO_STREAM_SET_ERROR(S) \
- ((void)((S)-__modeflags |= __FLAG_ERROR))
+ ((void)((S)-__errno_value = errno, (S)-__modeflags |= __FLAG_ERROR))
#define __STDIO_STREAM_CLEAR_EOF(S) \
((void)((S)-__modeflags = ~__FLAG_EOF))
diff --git a/libc/stdio/clearerr.c b/libc/stdio/clearerr.c
index a96ecaa..af1d09f 100644
--- a/libc/stdio/clearerr.c
+++ b/libc/stdio/clearerr.c
@@ -7,6 +7,41 @@
#include _stdio.h
+static inline void discard_buffered_data(FILE *stream)
+{
+#ifdef __STDIO_BUFFERS
+ /* Puty, but fpurge is rather non-portable,
+* thus many apps won't use it. Let them be able to clear
+* buffered data on write errors using clearerr().
+*/
+ if (__FERROR_UNLOCKED(stream) /* was there indeed an error? */
+ __STDIO_STREAM_IS_WRITING(stream)
+ stream-__errno_value != 0
+ stream-__errno_value != EINTR
+ stream-__errno_value != EAGAIN
+/* add more non-fatal error codes here */
+ ) {
+ stream-__bufpos = stream-__bufstart;
+ /*
+* Prevent spurious buffer resets on superfluous clearerrs.
+* Example:
+*
+* close(fileno(stdout));
+* printf(Hi there!\n); // error happens here, data is
buffered
+* dup2(good_fd, fileno(stdout));
+* clearerr(stdout); // drops buffered data
+* printf(Hi there!); // buffers new data
+* clearerr(stdout); // must NOT drop newly buffered data
+*
+* We assure this by __FERROR_UNLOCKED check above,
+* and by clearing __errno_value below
+* (yes, I am a bit paranoid).
+*/
+ stream-__errno_value = 0;
+ }
+#endif
+}
+
#undef clearerr
#ifdef __DO_UNLOCKED
@@ -15,6 +50,8 @@ void clearerr_unlocked(register FILE *stream)
{
__STDIO_STREAM_VALIDATE(stream);
+ discard_buffered_data(stream);
+
__CLEARERR_UNLOCKED(stream);
}
@@ -32,6 +69,8 @@ void clearerr(register FILE *stream)
__STDIO_STREAM_VALIDATE(stream);
+ discard_buffered_data(stream);
+
__CLEARERR_UNLOCKED(stream);
__STDIO_AUTO_THREADUNLOCK(stream);
diff --git a/libc/stdio/fflush.c b/libc/stdio/fflush.c
index d9104a4..4599b58 100644
--- a/libc/stdio/fflush.c
+++ b/libc/stdio/fflush.c
@@ -134,8 +134,8 @@ int fflush_unlocked(register FILE *stream)
* shouldn't flush a stream you were reading from. As usual,
glibc
* caters to broken programs and simply ignores this. */
__UNDEFINED_OR_NONPORTABLE;
-
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Sunday 11 March 2012 18:10, Rich Felker wrote:
On Sun, Mar 11, 2012 at 04:12:19PM +0100, Denys Vlasenko wrote:
On Sunday 11 March 2012 14:46, Denys Vlasenko wrote:
I propose the following patch.
Only compile tested by me, care to run-test?
A more correct (I hope) version:
I'm curious why this is such a big patch introducing new functions and
struct members. Couldn't it be just a one-line change in the function
that writes out the buffers (i.e. do the buffer-voiding as soon as the
error is hit rather than testing for it later)?
EINTR and EAGAIN should not result in output buffers being dropped.
You need to remember the error code for this.
Perhaps I'm missing
something about uclibc's design that makes this not work well, but
it's how my implementation in musl has always done it and I could
switch to the bad glibc behavior just by changing one line...
How about this simpler one?
--- a/libc/stdio/_WRITE.c
+++ b/libc/stdio/_WRITE.c
@@ -58,13 +58,29 @@ size_t attribute_hidden __stdio_WRITE(register FILE *stream,
todo -= rv;
buf += rv;
} else
-#ifdef __UCLIBC_MJN3_ONLY__
-#warning EINTR?
-#endif
-/* if (errno != EINTR) */
- {
- __STDIO_STREAM_SET_ERROR(stream);
+ __STDIO_STREAM_SET_ERROR(stream);
+
+ /* We buffer data on transient errors, but discard it
+* on hard ones. Example of a hard error:
+*
+* close(fileno(stdout));
+* printf(Hi there 1\n); // EBADF
+* dup2(good_fd, fileno(stdout));
+* printf(Hi there 2\n); // buffers new data
+*
+* This program should not print Hi there 1 to good_fd.
+* The rationale is that the caller of writing operation
+* should check for error and act on it.
+* If he didn't, then future users of the stream
+* have no idea what to do.
+* It's least confusing to at least not burden them with
+* some hidden buffered crap in the buffer.
+*/
+ if (errno == EINTR
+|| errno == EAGAIN
+/* do we have other soft errors? */
+ ) {
#ifdef __STDIO_BUFFERS
stodo = __STDIO_STREAM_BUFFER_SIZE(stream);
if (stodo != 0) {
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
On Sat, Feb 12, 2011 at 6:31 PM, Denys Vlasenko
vda.li...@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).
This seems to be causing some strange problems on bash as well. Is
there any interest in trying to mimic the glibc behavior?
uClibc version is 0.9.32.1.
bash problem:
# bash --version
GNU bash, version 3.2.0(1)-release (mipsel-unknown-linux-gnu)
Copyright (C) 2005 Free Software Foundation, Inc.
# echo none of the above /sys/power/state
sh: echo: write error: Invalid argument
# a
sh: a: command not found
none of the abov# b
sh: b: command not found
none of the abov#
# echo mem /sys/power/state
sh: echo: write error: Invalid argument
# echo mem /sys/power/state
sh: echo: write error: Invalid argument
# busybox echo mem /sys/power/state
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.01 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
Suspending console(s) (use no_console_suspend to debug)
Simple test case to show that the buffered data persists (albeit in
truncated form) even after calling fflush() and clearerr():
-- 8 --
#include stdio.h
#include unistd.h
#include fcntl.h
int main(int argc, char **argv)
{
int tmpfd, fd;
tmpfd = fcntl(fileno(stdout), F_DUPFD, 0);
fd = open(/dev/null, O_RDONLY);
dup2(fd, fileno(stdout));
printf(hello world\n);
fflush(stdout);
if (ferror(stdout))
clearerr(stdout);
dup2(tmpfd, fileno(stdout));
printf(goodbye world\n);
return 0;
}
-- 8 --
Output:
# ./test
hello worlgoodbye world
#
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] prevent retries on fclose/fflush after write errors
Denys Vlasenko vda.li...@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.
--
vda
diff -d -urpN uClibc.0/libc/stdio/_wcommit.c
uClibc.1/libc/stdio/_wcommit.c
--- uClibc.0/libc/stdio/_wcommit.c 2011-02-07 00:04:34.0 +0100
+++ uClibc.1/libc/stdio/_wcommit.c 2011-02-07 00:55:24.0 +0100
@@ -20,7 +20,18 @@ size_t attribute_hidden __stdio_wcommit(
__STDIO_STREAM_VALIDATE(stream);
- if ((bufsize = __STDIO_STREAM_BUFFER_WUSED(stream)) != 0) {
+ /* Note: we do not write anything if write error has been detected.
+ * Otherwise, stdio user has no way to prevent retries after
+ * failed write - and some users do want to not have any retries!
+ * IOW: if write errored out, neither fflush nor fclose should
+ * try to write buffered data.
+ * clearerr may be used to enable retry if needed.
+ */
+
+ bufsize = __STDIO_STREAM_BUFFER_WUSED(stream);
+ if (bufsize != 0
+!(stream-__modeflags __FLAG_ERROR)
+ ) {
stream-__bufpos = stream-__bufstart;
__stdio_WRITE(stream, stream-__bufstart, bufsize);
}
Hi,
Sounds plausible (but I did not check the standard), please install.
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
