Re: [PATCH] strerror_r: simplify AIX code.

[Eric Blake]

On 05/20/2011 04:50 PM, Bruno Haible wrote:
> Hi Eric,
> 
>> @@ -95,6 +95,15 @@ int
>>  strerror_r (int errnum, char *buf, size_t buflen)
>>  #undef strerror_r
>>  {
>> +  /* Filter this out now, so that rest of this replacement knows that
>> +     there is room for a non-empty message and trailing NUL.  */
>> +  if (buflen <= 1)
>> +    {
>> +      if (buflen)
>> +        *buf = 0;
>> +      return ERANGE;
>> +    }
>> +
>>  #if GNULIB_defined_ETXTBSY \
>>      || GNULIB_defined_ESOCK \
>>      || GNULIB_defined_ENOMSG \
> 
> This is odd and inconsistent.
>   errnum = -3, buflen >= 2 -> return EINVAL
>   errnum = -3, buflen <= 1 -> return ERANGE

But both errors are allowed by POSIX, and we have the case that glibc's
__xpg_strerror_r favors EINVAL while cygwin's _xpg_strerror_r favors
ERANGE.  The testsuite tests for both codes, because there is no
heirarchy in POSIX on which error code if more than one error code
simultaneously applies.

I can go with an additional restriction in the testsuite to _always_
favor ERANGE over EINVAL, but that would mean that glibc's
__xpg_strerror_r would also be replaced; and our goal has typically been
to avoid replacing glibc if it already complies with POSIX.

> 
> Also it seems strange to introduce new code for all platforms,
> from FreeBSD to Cygwin, with the rationale that it's a simplification
> for AIX.

Yes, but it _also_ simplifies my next patch, now posted, which touches
glibc, cygwin, bsd, hp-ux, and solaris code; several of which all
benefitted by having buflen of 1 already filtered out in advance.

> 
> I would not have done this. Rather, since the caller does not know
> in advance how long the string will be (i.e. how long the buffer needs
> to be for a complete message), that ERANGE should be the "lowest priority"
> error. That is, I think EINVAL should take precedence over ERANGE.

I personally disagree - if I call strerror_r(-12, buf, 17) and get
ERANGE, then I immediately know that "Unknown error -1" is suspect;  but
if I get EINVAL, then I've just printed a misleading error number, and
the only way to tell if I didn't suffer from truncation is to retry with
a larger buffer until I get an answer with a strlen() smaller than my
buffer.  That is, if I am going to _guarantee_ that all errors produce a
non-empty message, I'd much rather prefer learning that my buffer was
too small until I got my complete non-empty message.

> snprintf:
> 
>   This function overwrites memory even when a size argument of 1 is passed on 
> some
>   platforms:
>   Linux libc5.

Yep - early exit on buflen <= 1 is what _lets_ me use snprintf in the
next patch, without having to drag in the snprintf module.

> 
> then it is because these system functions were coded with the "let's check the
> size upfront" philosophy. And now you introduce the same, broken idiom here?

If you _guarantee_ a non-empty string, then there are only two lengths
that cannot produce such a string: 0 and 1.  And those are the two where
we do the early exit, and avoid all the shenanigans of calling the
system's strerror just to find out that we'd be returning an error and
an empty string anyways.

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature