kern/164674: vsprintf/vswprintf return error (EOF) on success if
__SERR flag is already set on file
Matthew Story
matthewstory at gmail.com
Mon Apr 2 20:05:25 UTC 2012
[excerpts from other thread]
On Wed, Mar 21, 2012 at 5:45 PM, David Schultz <das at freebsd.org> wrote:
> On Mon, Mar 12, 2012, Matthew Story wrote:
>> On Sun, Mar 4, 2012 at 2:03 PM, John Baldwin <jhb at freebsd.org> wrote:
>>
>> > My standard-parsing fu is weak. Can some other folks on this list
look at
>> > this PR, figure out what the correct semantics for *fprintf() in this
case,
>> > and evaluate the patch? Thanks.
> [...]
>> libc vfprintf currently behaves differently on un|buffered FILEs with
>> ferror set:
>>
>> 1. Unbuffered FILEs
>>
>> -> does not transmit any bytes, and returns -1
>>
>> 2. Buffered FILEs (fully buffered, or line buffered)
>>
>> -> transmits all bytes, and returns -1
>>
>> I believe that this should be reconciled such that vfprintf (and all
>> derivatives) returns sucess|failure based on transmission, not taking
>> ferror state into account, but an alternate solution would be to
reconcile
>> the behavior of buffered FILEs with the current behavior of unbuffered
>> FILEs (e.g. detect ferror state, transmit no bytes and return -1).
>
> I agree with this assessment. When the error indicator is set on
> a stream, it means that an error has occurred at some point---not
> that all subsequent operations on the stream should fail.
>
> There ought to be a less ugly fix than the one proposed. Probably
> the PRINT macro and the various other evil macros in vfprintf()
> should set ret to EOF, and the following lines in vfprintf.c should
> be removed:
>
> if (__sferror(fp))
> ret = EOF;
>
> If vfprintf() is fixed so that printing to a buffered stream
> always returns success after a successful write (regardless of the
> prior state of the stream's error indicator), that should fix the
> problem for unbuffered streams automatically. Unbuffered streams
> go through __sbprintf(), which throws away the output if
> vfprintf() returns -1.
I've followed up on David's suggestion to remove this test and set ret =
EOF prior to goto error in all relevant places. I audited all cases where
__SERR could potentially be set, and determined that all cases are
correctly setting __SERR, returning error in the bounding function, and
properly going to error within __vf(w)printf already (I have an exhaustive
list of these if anyone is curious ...).
Original test program in the PR works with this patch, and the regressions
for stdio all function as expected. I took the liberty of cleaning up a
few return -1's, in favor of return EOF's.
All in all, this is much less ugly than the previous solution. patch is
against head ...
--
regards,
matt
-------------- next part --------------
Index: lib/libc/stdio/vfwprintf.c
===================================================================
--- lib/libc/stdio/vfwprintf.c (revision 233780)
+++ lib/libc/stdio/vfwprintf.c (working copy)
@@ -449,20 +449,28 @@
/* BEWARE, these `goto error' on error. */
#define PRINT(ptr, len) do { \
- if (io_print(&io, (ptr), (len), locale)) \
- goto error; \
+ if (io_print(&io, (ptr), (len), locale)) { \
+ ret = EOF; \
+ goto error; \
+ } \
} while (0)
#define PAD(howmany, with) { \
- if (io_pad(&io, (howmany), (with), locale)) \
- goto error; \
+ if (io_pad(&io, (howmany), (with), locale)) { \
+ ret = EOF; \
+ goto error; \
+ } \
}
#define PRINTANDPAD(p, ep, len, with) { \
- if (io_printandpad(&io, (p), (ep), (len), (with), locale)) \
- goto error; \
+ if (io_printandpad(&io, (p), (ep), (len), (with), locale)) { \
+ ret = EOF; \
+ goto error; \
+ } \
}
#define FLUSH() { \
- if (io_flush(&io, locale)) \
- goto error; \
+ if (io_flush(&io, locale)) { \
+ ret = EOF; \
+ goto error; \
+ } \
}
/*
@@ -897,6 +905,7 @@
convbuf = __mbsconv(mbp, prec);
if (convbuf == NULL) {
fp->_flags |= __SERR;
+ ret = EOF;
goto error;
}
cp = convbuf;
@@ -1030,8 +1039,10 @@
/* leading zeroes from decimal precision */
PAD(dprec - size, zeroes);
if (gs.grouping) {
- if (grouping_print(&gs, &io, cp, buf+BUF, locale) < 0)
+ if (grouping_print(&gs, &io, cp, buf+BUF, locale) < 0) {
+ ret = EOF;
goto error;
+ }
} else {
PRINT(cp, size);
}
@@ -1047,10 +1058,11 @@
prec += expt;
} else {
if (gs.grouping) {
- n = grouping_print(&gs, &io,
- cp, convbuf + ndig, locale);
- if (n < 0)
+ if ((n = grouping_print(&gs, &io,
+ cp, convbuf + ndig, locale)) < 0) {
+ ret = EOF;
goto error;
+ }
cp += n;
} else {
PRINTANDPAD(cp, convbuf + ndig,
@@ -1089,8 +1101,6 @@
va_end(orgap);
if (convbuf != NULL)
free(convbuf);
- if (__sferror(fp))
- ret = EOF;
if ((argtable != NULL) && (argtable != statargtable))
free (argtable);
return (ret);
Index: lib/libc/stdio/vfprintf.c
===================================================================
--- lib/libc/stdio/vfprintf.c (revision 233780)
+++ lib/libc/stdio/vfprintf.c (working copy)
@@ -127,7 +127,7 @@
const CHAR *cp0 = cp;
if (io_printandpad(iop, cp, ep, gs->lead, zeroes, locale))
- return (-1);
+ return (EOF);
cp += gs->lead;
while (gs->nseps > 0 || gs->nrepeats > 0) {
if (gs->nrepeats > 0)
@@ -137,9 +137,9 @@
gs->nseps--;
}
if (io_print(iop, gs->thousands_sep, gs->thousep_len, locale))
- return (-1);
+ return (EOF);
if (io_printandpad(iop, cp, ep, *gs->grouping, zeroes, locale))
- return (-1);
+ return (EOF);
cp += *gs->grouping;
}
if (cp > ep)
@@ -369,20 +369,28 @@
/* BEWARE, these `goto error' on error. */
#define PRINT(ptr, len) { \
- if (io_print(&io, (ptr), (len), locale)) \
- goto error; \
+ if (io_print(&io, (ptr), (len), locale)) { \
+ ret = EOF; \
+ goto error; \
+ } \
}
#define PAD(howmany, with) { \
- if (io_pad(&io, (howmany), (with), locale)) \
+ if (io_pad(&io, (howmany), (with), locale)) { \
+ ret = EOF; \
goto error; \
+ } \
}
#define PRINTANDPAD(p, ep, len, with) { \
- if (io_printandpad(&io, (p), (ep), (len), (with), locale)) \
- goto error; \
+ if (io_printandpad(&io, (p), (ep), (len), (with), locale)) { \
+ ret = EOF; \
+ goto error; \
+ } \
}
#define FLUSH() { \
- if (io_flush(&io, locale)) \
+ if (io_flush(&io, locale)) { \
+ ret = EOF; \
goto error; \
+ } \
}
/*
@@ -617,6 +625,7 @@
(wchar_t)GETARG(wint_t), &mbs);
if (mbseqlen == (size_t)-1) {
fp->_flags |= __SERR;
+ ret = EOF;
goto error;
}
size = (int)mbseqlen;
@@ -828,6 +837,7 @@
convbuf = __wcsconv(wcp, prec);
if (convbuf == NULL) {
fp->_flags |= __SERR;
+ ret = EOF;
goto error;
}
cp = convbuf;
@@ -962,8 +972,10 @@
/* leading zeroes from decimal precision */
PAD(dprec - size, zeroes);
if (gs.grouping) {
- if (grouping_print(&gs, &io, cp, buf+BUF, locale) < 0)
+ if (grouping_print(&gs, &io, cp, buf+BUF, locale) < 0) {
+ ret = EOF;
goto error;
+ }
} else {
PRINT(cp, size);
}
@@ -979,10 +991,11 @@
prec += expt;
} else {
if (gs.grouping) {
- n = grouping_print(&gs, &io,
- cp, dtoaend, locale);
- if (n < 0)
+ if ((n = grouping_print(&gs, &io,
+ cp, dtoaend, locale)) < 0) {
+ ret = EOF;
goto error;
+ }
cp += n;
} else {
PRINTANDPAD(cp, dtoaend,
@@ -1024,8 +1037,6 @@
#endif
if (convbuf != NULL)
free(convbuf);
- if (__sferror(fp))
- ret = EOF;
if ((argtable != NULL) && (argtable != statargtable))
free (argtable);
return (ret);
More information about the freebsd-standards
mailing list