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

[Roar Bjørgum Rotvik]

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.

You indicate that errno may be clobbered by OLSR_PRINTF() or perror(),
but look at this description from "man 3 perror" on linux:
"The error number is taken  from  the external variable errno, which is
set when errors occur but not cleared when non-erroneous calls are made."

That means that if a system call returns error code (normally -1), then
errno is set to new error value. If the functions do not fail and return
an error code, the errno value is unchanged.

And by looking at the man page for fprintf (as OLSR_PRINTF uses), it may
return an negative value but the man page does not specify that the
errno is changed or set to a spesific value.
Compare that to the man page of an system call that is known to change
the errno value, like the read() call:
"RETURN VALUE
  ...
  On error, -1 is returned, and errno is set appropriately. In this case
it is left unspecified whether the file position (if any) changes."

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 :)

Some more comments to the code is embedded below:

> --- src/olsr.c.orig	2006-01-05 17:28:44.000000000 +0100
> +++ src/olsr.c	2006-01-05 16:51:36.000000000 +0100
> @@ -606,9 +606,10 @@
>  
>    if((ptr = malloc(size)) == 0) 
>      {
> -      OLSR_PRINTF(1, "OUT OF MEMORY: %s\n", strerror(errno))
> -      olsr_syslog(OLSR_LOG_ERR, "olsrd: out of memory!: %m\n");
> -      olsr_exit((char *)id, EXIT_FAILURE);
> +      const char * const errmsg = strerror(errno);
> +      OLSR_PRINTF(1, "OUT OF MEMORY: %s\n", errmsg)
> +        olsr_syslog(OLSR_LOG_ERR, "olsrd: out of memory!: %s\n", errmsg);
> +      olsr_exit(id, EXIT_FAILURE);
>      }
>    return ptr;
>  }

And here, refer to my comments above; I think it is not needed to assign
the errno string to a variable first as OLSR_PRINTF() does not change
errno. Could do it like this:

> --- src/olsr.c.orig	2006-01-05 17:28:44.000000000 +0100
> +++ src/olsr.c	2006-01-05 16:51:36.000000000 +0100
> @@ -606,9 +606,10 @@
>
>    if((ptr = malloc(size)) == 0)
>      {
> -      OLSR_PRINTF(1, "OUT OF MEMORY: %s\n", strerror(errno))
> -      olsr_syslog(OLSR_LOG_ERR, "olsrd: out of memory!: %m\n");
> -      olsr_exit((char *)id, EXIT_FAILURE);
> +      OLSR_PRINTF(1, "OUT OF MEMORY: %s\n", strerror(errno))
> +      olsr_syslog(OLSR_LOG_ERR, "olsrd: out of memory!: %s\n",
                     strerror(errno));
> +      olsr_exit(id, EXIT_FAILURE);
>      }
>    return ptr;
>  }

-- 
Roar Bjørgum Rotvik