This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: strerror_r questions


On 02/09/2011 03:11 PM, Corinna Vinschen wrote:
>> Well, for backwards compatibility (as well as glibc compatibility), I
>> thought it better we still get the _GNU_SOURCE version by default;

I stand corrected on glibc defaults:

$ printf '#include <string.h>\n'|gcc -D_POSIX_C_SOURCE=200112L -E - \
  | grep strerror_r
extern int strerror_r (int __errnum, char *__buf, size_t __buflen)
__asm__ ("" "__xpg_strerror_r") __attribute__ ((__nothrow__))

$ printf '#include <string.h>\n'|gcc -D_GNU_SOURCE=1 -E - \
  | grep strerror_r
extern char *strerror_r (int __errnum, char *__buf, size_t __buflen)
     __attribute__ ((__nothrow__)) __attribute__ ((__nonnull__ (2)));

$ printf '#include <string.h>\n'|gcc -E - | grep strerror_r
extern int strerror_r (int __errnum, char *__buf, size_t __buflen)
__asm__ ("" "__xpg_strerror_r") __attribute__ ((__nothrow__))

So I'll fix that up.

>> +#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?

Oops, yes.  Will fix in v2.

> 
>> @@ -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.

The "Unknown error XXX" message is a cygwin artifact, not a newlib
artifact.  That is, cygwin should be using it's own strerror_r, since it
provides its own strerror() with different behavior than newlib's.

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

No.  I strongly feel that strerror and strerror_r should return the same
strings (insofar as possible).  However, newlib's strerror() returns "",
not "Unknown error XXX", for an unknown string, for three reasons:
1. strerror_r must be thread-safe, but it calls strerror, so strerror
must also be thread-safe in newlib (in general, POSIX does not require
strerror to be thread-safe, but in that case, strerror_r cannot be
implemented by calling strerror); newlib achieves its thread-safety by
returning only constant strings (well, newlib also provides the
_user_sterror hook, which must be thread-safe for all this to work)
2. newlib did not set aside any thread-local storage for computing an
alternate string, and I don't want to change struct REENT to add such
storage
3. newlib targets embedded systems, and dragging in snprintf() to
compute an alternate string is bloat.

Meanwhile, cygwin's overrides newlib's strerror() with its own version,
and cygwin's version gets away with a thread-safe "Unknown error XXX",
because it uses thread-local storage, and because it does not have the
snprintf executable size concerns of embedded systems.  That just means
that this newlib patch now means that cygwin has to also provide its own
sterror_r replacement, rather than relying on newlib, to match the fact
that it already provides its own strerror() replacement.

>   else if (n > 0)
>     {
>       buffer[0] = '\0';
>       strncat (buffer, error, n - 1);
>     }
>   return buffer;

This truncates the message.  But glibc's behavior is that even when n
was too small, you get the untruncated message.  How?  By returning the
original (thread-safe) message from strerror instead of the user's
pointer.  The glibc docs state merely that the return value is
NUL-terminated, not that buf is also NUL-terminated (which, for n==0, is
impossible).


>> +_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.

Back to the idea that strerror_r should give the same output as
strerror, are you saying that I should add thread-local storage to REENT
to provide the "unknown error" details instead of ""?  Newlib's
implementation, while not ideal, is at least compliant, without making
it more complex to deal with thread-local computed strings.

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]