Re: [PATCH 13/13] string: Add errname_np and errdesc_np

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 28/05/2020 09:28, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> The errname_np returns error number name (i.g. "EINVAL" for EINVAL)
>> while errdescr_np returns string describing error number (i.g
>> "Invalid argument" for EINVAL).  Different than strerror, errdescr_np
>> does not attempt to translate the return description and both functions
>> return NULL for an invalid error number.
> 
> If you use a name that starts with "str", it's in the implementation
> namespace, and you don't need the _np suffix.

I don't have a strong preference here, maybe 'strerror_as'?

> 
> Is errdescr_np really that useful?  If yes, maybe we should define a
> global C locale object, similar to LC_GLOBAL_LOCALE, so that programmers
> can simply use strerror_l for this instead (except that it won't be
> async-signal-safe, hmm)?

The errdescr_np is added to provide the async-signal-safe strerror 
analogous of the patch deprecated sys_errlist.  The libSegfault itself
could use it, although I am not sure if async-safeness is really that
useful. It is way simpler than define a extra global C locale though.

> 
>> diff --git a/manual/errno.texi b/manual/errno.texi
>> index 8cb4ce8b48..97ba3916ed 100644
>> --- a/manual/errno.texi
>> +++ b/manual/errno.texi
>> @@ -1207,6 +1207,28 @@ to @code{errno}.
>>  The function @code{perror} is declared in @file{stdio.h}.
>>  @end deftypefun
>>  
>> +@deftypefun void errname_np (int @var{errnum})
>> +@standards{GNU, string.h}
>> +@safety{@prelim{}@mtsafe{@mtssigintr{}}@assafe{}@acsafe{}}
>> +This function returns the name describing the error @var{errnum} or
>> +@code{NULL} for invalid error number (i.g "EINVAL" for @code{EINVAL}).
>> +
>> +@pindex string.h
>> +This function is a GNU extension, declared in the header file @file{string.h}.
>> +@end deftypefun
>> +
>> +@deftypefun void errdesc_np (int @var{errnum})
>> +@standards{GNU, string.h}
>> +@safety{@prelim{}@mtsafe{@mtssigintr{}}@assafe{}@acsafe{}}
>> +This function returns the message describing the error @var{errnum} or
>> +@code{NULL} for invalid error number (i.g Invalid argument" for
>> +@code{EINVAL}).  Different than @code{strerror} the returned description is
>> +not translated.
>> +
>> +@pindex string.h
>> +This function is a GNU extension, declared in the header file @file{string.h}.
>> +@end deftypefun
> 
> Why @mtssigintr{} (twice)?

I am not following, each one annotates a different symbol (one for errname_np
and another for errdesc_np).

> 
> I think error numbers for which no constant exist are still valid (in
> the C++ sense), so maybe we should phrase this differently: These
> functions return NULL if there is no known E* constant with this value.

Ack.

> 
>> diff --git a/stdio-common/errlist.c b/stdio-common/errlist.c
>> index f7a49fbf35..dd078143f2 100644
>> --- a/stdio-common/errlist.c
>> +++ b/stdio-common/errlist.c
>> @@ -28,4 +28,12 @@ const char *const _sys_errlist_internal[] =
>>  
>>  const int _sys_nerr_internal = array_length (_sys_errlist_internal);
>>  
>> +const char *const _sys_errname_internal[] =
>> +  {
>> +/* This file is auto-generated from errlist.def.  */
>> +#include <errlist-name.h>
>> +  };
>> +
>> +const int _sys_nname_internal = array_length (_sys_errname_internal);
>> +
>>  #include <errlist-compat.c>
> 
> There should not be a reason to emit a separate constant for the length
> to the data segment.  And we should eliminate the extra relocations this
> array adds.  But I think we can add these changes later as an
> optimization.

Alright, I think it could be feasible to auto-generate both
_sys_errlist_internal and _sys_errname_internal with its expected size
(as we do for __sys_siglist_internal) and optimize the script to
emit the expected string size for each element.

> 
>> diff --git a/stdio-common/test-err_np.c b/stdio-common/test-err_np.c
>> new file mode 100644
>> index 0000000000..b463c9f3ef
>> --- /dev/null
>> +++ b/stdio-common/test-err_np.c
> 
> I would expect this test to compare against strerror and the E*
> constants directly.  For the latter, we should generate a table
> containing elements like:
> 
>   { EINVAL, "EINVAL" },

The 'errname' does have this mapping. Should I add an extra test for
strerror as well?

>   
>> diff --git a/string/_strerror.c b/string/_strerror.c
>> index 6ae8d47f0f..8bb9bfeaa9 100644
>> --- a/string/_strerror.c
>> +++ b/string/_strerror.c
>> @@ -24,14 +24,14 @@
>>  char *
>>  __strerror_r (int errnum, char *buf, size_t buflen)
>>  {
>> -  if (__glibc_unlikely (errnum < 0 || errnum >= _sys_nerr_internal
>> -			|| _sys_errlist_internal[errnum] == NULL))
>> +  const char *r = __errdescr_np (errnum);
>> +  if (r == NULL)
>>      {
>>        __snprintf (buf, buflen, "%s%d", _("Unknown error "), errnum);
>>        return buf;
>>      }
>>  
>> -  return (char *) _(_sys_errlist_internal[errnum]);
>> +  return _(_sys_errlist_internal[errnum]);
> 
> Shouldn't the last line re-use r?

It should, I have change it.

> 
>> diff --git a/string/errname_np.c b/string/errname_np.c
>> new file mode 100644
>> index 0000000000..61455da219
>> --- /dev/null
>> +++ b/string/errname_np.c
> 
>> +const char *const
>> +errname_np (int errnum)
>> +{
>> +  const char *name = NULL;
>> +
>> +  if (errnum >= 0 && errnum < _sys_nname_internal)
>> +    name = _sys_errname_internal[errnum];
>> +
>> +  return name;
>> +}
> 
> I'm surprised that index 0 is looked up in the table.

It is defined a no error ('Success').

> 
> It may make sense to move the definition of the array out ouf errlist.c,
> so that it can be shared across all ports.

On third patch on this set [1] I moved the generation to a script based
on errlist.def. Hurd requires its own version because it uses different
internal names (maybe we could cleanup this up later).

[1] https://sourceware.org/pipermail/libc-alpha/2020-May/113700.html

> 
> Thanks,
> Florian
>