Re: strerror_r questions

[Corinna Vinschen <vinschen at redhat dot com>]

Hi Eric,

On Feb  9 14:09, Eric Blake wrote:
> On 02/06/2011 02:52 AM, Corinna Vinschen wrote:
> > First of all, the existing implementation follows glibc's lead, so
> > it should stick to the API but the behavior should be fixed.
> > 
> > Other than that, the Linux approach would work best.  Provide both
> > interfaces and use test macros.  Fortunately they are documented in the
> > man page:
> > 
> >   The XSI-compliant version of strerror_r() is provided if:
> >   (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE
> >   Otherwise, the GNU-specific version is provided.
> > 
> > Ideally we don't define _GNU_SOURCE by default, so this should result in
> > getting the POSIX version by default.
> 
> Well, for backwards compatibility (as well as glibc compatibility), I
> thought it better we still get the _GNU_SOURCE version by default; but I
> can tweak things if you really want me to default to POSIX when no
> feature test macros are present.  At any rate, here's the newlib side of
> the patch (cygwin will need to be patched separately).

My POV is, Linux compatibility is a nice feature, but if glibc and POSIX
disagree, then POSIX should have precedent.  It's the standard.  Anyway,
I'm not adamant about it, and backward-compatibility is certainly a good
point as well.

> --- a/newlib/libc/include/string.h
> +++ b/newlib/libc/include/string.h
> @@ -66,7 +66,22 @@ char 	*_EXFUN(strdup,(const char *));
>  char 	*_EXFUN(_strdup_r,(struct _reent *, const char *));
>  char 	*_EXFUN(strndup,(const char *, size_t));
>  char 	*_EXFUN(_strndup_r,(struct _reent *, const char *, size_t));
> -char 	*_EXFUN(strerror_r,(int, char *, size_t));
> +/* There are two common strerror_r variants.  If you request
> +   _GNU_SOURCE, you get the GNU version; otherwise if you request
> +   standards compliance, you get the POSIX version; otherwise (for
> +   backwards compatibility) you get the GNU version.  POSIX requires
> +   that #undef strerror_r will still let you invoke the underlying
> +   function, but we only support that with gcc.  */
> +#if (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !_GNU_SOURCE
> +# ifdef __GNUC__
> +char    *_EXFUN(strerror_r,(int, char *, size_t)) __asm__ ("__xpg_strerror_r");

Erm... shouldn't the return value be "int" rather than "char *" in this
case?

> @@ -49,5 +64,6 @@ _DEFUN (strerror_r, (errnum, buffer, n),
>    char *error;
>    error = strerror (errnum);
> 
> -  return strncpy (buffer, (const char *)error, n);
> +  strncpy (buffer, error, n);
> +  return strlen (error) >= n ? error : buffer;

This looks wrong to me.  Glibc guarantees that the copied result in
buffer is NUL-terminated, and I'm missing the "Unknown error XXX"
message.  Wern't these problems the main reason for this patch?  So,
shouldn't that be rather something like this?

  if (strlen (error) >= n)
    return error;
  if (!*error)
    snprintf (buffer, n, "Unknown error %d", errnum);
  else if (n > 0)
    {
      buffer[0] = '\0';
      strncat (buffer, error, n - 1);
    }
  return buffer;


> +_DEFUN (__xpg_strerror_r, (errnum, buffer, n),
> +	int errnum _AND
> +	char *buffer _AND
> +	size_t n)
> +{
> +  char *error;
> +  error = strerror (errnum);
> +
> +  strncpy (buffer, error, n);

Same here.  POSIX does not require that the string in buffer is
NUL-terminated, that's what the ERANGE is for, but the POSIX man page
also contains words about unknown error codes as a [CX] type extension:

  "If the value of errnum is a valid error number, the message string
   shall indicate what error occurred; otherwise, if these functions
   complete successfully, the message string shall indicate that an
   unknown error occurred."

I think it makes sense to implement this as well.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat