[PATCH] printf: fix exit code on conversion failure

[Denys Vlasenko]

On Thursday 18 June 2009 22:39, Colin Watson wrote:
> On Thu, Jun 18, 2009 at 10:21:37PM +0200, Denys Vlasenko wrote:
> > On Thursday 18 June 2009 17:52, Colin Watson wrote:
> > > @@ -69,7 +67,9 @@ static int multiconvert(const char *arg, void *result, converter convert)
> > >  	errno = 0;
> > >  	convert(arg, result);
> > >  	if (errno) {
> > > +		int save_errno = errno;
> > >  		bb_error_msg("%s: invalid number", arg);
> > > +		errno = save_errno;
> > >  		return 1;
> > >  	}
> > 
> > Why?
> 
> Using stdio, syslog, etc. is not guaranteed to preserve errno. If you
> want to keep it reliably, you have to save it. Relying on its value
> across calls to those functions is courting unspecified behaviour.

> http://www.opengroup.org/onlinepubs/009695399/functions/errno.html
> states that "The setting of errno after a successful call to a function
> is unspecified unless the description of that function specifies that
> errno shall not be modified", and e.g. neither fflush nor syslog
> specifies this.

Well, it may change, but it never goes to 0:

"No function in this volume of IEEE Std 1003.1-2001 shall set errno to 0."

And that's the only thing we care about: errno != 0 -> bad.


> > Generally, good idea.
> > 
> > I propose this modification.
> 
> My change to multiconvert should be kept for the reasons given above.
> 
> My changes to print_direc should be kept (with the pointer parameter ->
> return value change you suggested) because print_direc calls printf all
> over the place and printf is not guaranteed to preserve errno on
> success.

So you are saying that successful printf may set errno to nonzero?!

POSIX manpage says:

ERRORS
       For the conditions under which fprintf()  and  printf()  fail  and  may
       fail, refer to fputc() or fputwc() .

       In addition, all forms of fprintf() may fail if:

       EILSEQ A  wide-character code that does not correspond to a valid char-
              acter has been detected.
       EINVAL There are insufficient arguments.

       The printf() and fprintf() functions may fail if:
       ENOMEM Insufficient storage space is available.

Where do you think they mean they store EINVAL etc?
Obviously, errno.


There is a reason why I want to simplify the code:

Your patch:

# make bloatcheck
function                                             old     new   delta
print_direc                                          427     481     +54
printf_main                                          834     842      +8
multiconvert                                          79      82      +3
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 3/0 up/down: 65/0)               Total: 65 bytes
   text    data     bss     dec     hex filename
 820667     458    6956  828081   ca2b1 busybox_old
 820732     458    6956  828146   ca2f2 busybox_unstripped

My adaptation of your patch:

# make bloatcheck
function                                             old     new   delta
printf_main                                          834     827      -7
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-7)               Total: -7 bytes
   text    data     bss     dec     hex filename
 820667     458    6956  828081   ca2b1 busybox_old
 820660     458    6956  828074   ca2aa busybox_unstripped

--
vda