Die "%m", die (was Re: [olsr-dev] [PATCH] Improvements in dot_draw plugin)

[Roar Bjørgum Rotvik]

Bernd Petrovitsch wrote:
> On Fri, 2006-01-06 at 09:54 +0100, Roar Bjørgum Rotvik wrote:
> 
>>Bernd Petrovitsch wrote:
>>
>>>On Mon, 2005-11-14 at 22:02 +0100, Thomas Lopatic wrote:
>>
>>[ ... ]
>>
>>>The attached patch against cvs-current kills all %m which are used for
>>>printf(), syslog() and the like and replaces them with the above
>>>solution.
>>>
>>>I also replaced the ones in the Unix-specific directories because
>>>- to be able to check if with a simple 
>>>find -type f -name '*.[ch]' -print0 | xargs -0r fgrep %m /dev/null
>>>- no one should be tempted or able to copy-paste them into "wrong"
>>>  files.
>>>- it actually fixes bugs since several of them are called after
>>>  e.g. OLSR_PRINTF() or perror() and they surely clobber "errno"
>>>  anyway.
>>
>>I have some comments to this patch.
> 
> Good, than someone actually reads it;-)
> 
>>Neither the manpages for fprintf() or perror() says that errno is set if
>>an error occours, so I don't think that errno is clobbered by these
>>functions. Do you know otherwise since you said "they surely clobber
>>"errno" anyway"?
>>
>>If you can prove me wrong I'll accept that :)
> 
> It is not a prove in the mathematical sense but:
> First you are of course that the manual pages do not explicitly mention
> the possible "destruction" of errno.
> However, IMHO chances are that fprintf(), perror() and cousins may use a
> sys-call like write(2).
> And since it is also not explicitly mentioned that errno is preserved
> (and at least for the perror() manual page I would assume that if
> perror(3) really does never change "errno" it would be mentioned), it
> seems safest to me to not assume some special behaviour.
> 
> At least uClibc's perror() function does use write(2).
> perror() in glibc-2.2.5 (sorry, that source was just handy) just calls
> fprintf(3) which simply calls vfprintf(3). And I assume now (since
> really delving into glibc source is somwhat cumbersome) that it may
> eventually use (in some libc implementation) write(2) or set "errno" for
> whatever else fails somewhere in between.

Ok, I did not investigate uClibc version and did not know it was using
write(). You are right that it should not be needed to investigate the
source code for system libraries to check if they change errno or not.

I'm convinced that calling perror(), fprintf() or similar may change
errno so the olsrd code should either store the error code/string as
your patch or just call one logging function on an error condition.

But a little note:
The error string from strerror() is a static string that may be changed
by either sterror() or perror(). From "man strerror":
"The strerror() function returns a string describing the error code
passed in the argument errnum, possibly using the LC_MESSAGES part of
the current locale to select the appropriate language.  This string must
not be modified by the application, but may be modified by a subsequent
call to perror() or strerror(). No library function will modify this
string."

Look at this example from your patch:
--- src/net_olsr.c.orig	2006-01-05 17:03:49.000000000 +0100
+++ src/net_olsr.c	2006-01-05 16:55:46.000000000 +0100
@@ -463,8 +463,9 @@
 		     sizeof (*sin))
 	 < 0)
 	{
+          const char * const errmsg = strerror(errno);
 	  perror("sendto(v4)");
-	  olsr_syslog(OLSR_LOG_ERR, "OLSR: sendto IPv4 %m");
+	  olsr_syslog(OLSR_LOG_ERR, "OLSR: sendto IPv4 %s", errmsg);
 	  netbufs[ifp->if_nr]->pending = 0;
 	  return -1;
 	}

Gives this code:
	if (olsr_sendto(...) < 0)
 	{
          const char * const errmsg = strerror(errno);
 	  perror("sendto(v4)");
	  olsr_syslog(OLSR_LOG_ERR, "OLSR: sendto IPv4 %s", errmsg);
 	  netbufs[ifp->if_nr]->pending = 0;
 	  return -1;
 	}

This means that the call to strerror() will store the error string from
sendto(), but as the man page for strerror() indicates, the call to
perror() may overwrite this static string and the call to olsr_syslog()
may in fact return error string from perror()... :)

What about storing the actual errno instead of the string as in this
example?
	if (olsr_sendto(...) < 0)
 	{
          const int my_errno = errno;
 	  perror("sendto(v4)");
	  olsr_syslog(OLSR_LOG_ERR, "OLSR: sendto IPv4 %s", strerror (my_errno));
 	  netbufs[ifp->if_nr]->pending = 0;
 	  return -1;
 	}
But then you need to call strerror(my_errno) as many times as you need
the string..

I'm not trying to be difficult, it's nice that someone creates patches
to fix bugs or add features. Just trying to avvoid any more bugs even if
the possibility for these bugs is very small :)

-- 
Roar Bjørgum Rotvik