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

[Florian Weimer <fweimer@redhat.com>]

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

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

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

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

> 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" },
  
> 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?

> 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 may make sense to move the definition of the array out ouf errlist.c,
so that it can be shared across all ports.

Thanks,
Florian