Re: [PATCH 1/1] libc: Added implementations and prototypes for

Hi Matt,


thanks for this implementation.  The patch looks bascically fine.
A few issues, though.

First, it's missing the Makefile.am entries to build the new files,
please add them.

On Jul 17 12:10, Matt Joyce wrote:
> Added implementations for sig2str() and str2sig() in libc/signal in order
> to improve POSIX compliance. Added function prototypes to sys/signal.h.
> ---
>  newlib/libc/include/sys/signal.h |  12 ++
>  newlib/libc/signal/sig2str.c     | 235 +++++++++++++++++++++++++++++++
>  2 files changed, 247 insertions(+)
>  create mode 100644 newlib/libc/signal/sig2str.c
> 
> diff --git a/newlib/libc/include/sys/signal.h b/newlib/libc/include/sys/signal.h
> index 45cc0366c..847dc59bd 100644
> --- a/newlib/libc/include/sys/signal.h
> +++ b/newlib/libc/include/sys/signal.h
> @@ -238,6 +238,18 @@ int sigqueue (pid_t, int, const union sigval);
>  
>  #endif /* __POSIX_VISIBLE >= 199309 */
>  
> +#if __GNU_VISIBLE
> +
> +/* POSIX Issue 8 adds sig2str() and str2sig(). */
> +
> +/* This allows for the max length of the error message and longest integer. */
> +#define SIG2STR_MAX sizeof("Unknown signal 4294967295 ")

While looking through your patch I realized that this request of mine
was nonsense.  The idea was that the buffers of length SIG2STR_MAX
have to store descriptive signal texts, like "Floating point exception"
or "Real-time signal 10", but that's not the case here anyway.

If SIG2STR_MAX isn't already defined, gnulib defines SIG2STR_MAX
basically as sizeof "SIGRTMAX" + sizeof (max numerical string of type
int).  This results in different SIG2STR_MAX values depending of sizeof
int being 2, 4, or 8.

Given we strive for supporting smaller targets we might want to express
this similary, just a bit simpler.  For one thing, do we ever want to
support more than 4 billion RT signals?  I guess not.  My suggestion would
be something like:

#if __STDINT_EXP(INT_MAX) > 0x7fff
#define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("4294967295") - 1)
#else
#define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("65535") - 1)
#endif

> +int
> +sig2str(int signum, char *str)
> +{
> +  const sig_name_and_num *sptr;
> +
> +  /* If signum falls in the real time signals range, the Issue 8 standard 
> +  * gives the option of defining the saved str value as either "RTMIN+n" or
> +  * "RTMAX-m".*/
> +  if ((SIGRTMIN + 1) <= signum && signum <= (SIGRTMAX -1)) {
                                                         ^^^
                                                         space

> +    sprintf(str, "RTMIN+%d", (signum-SIGRTMIN));
> +    return 0; 
> +  }

This is not entirely correct.  The POSIX draft requires the signal
string to be returned for RT signals to be expressed as either "RTMIN+x"
and "RTMAX-x", dependent on the signal number.  Please see
https://www.opengroup.org/austin/docs/austin_1110.pdf, page 85, the two
paragraphs starting at line 61678 (unfortunately copy/paste from this
doc doesn't work nicely).

> +int
> +str2sig(const char *restrict str, int *restrict pnum)
> +{
> +  int j = 0;
> +  const sig_name_and_num *sptr; 
> +  char dest[SIG2STR_MAX];
> +  int is_valid_decimal;
> +  is_valid_decimal = atoi(str);
> +
> +  /* If str is a representation of a decimal value, save its integer value
> +   * in pnum. */
> +  if (1 <= is_valid_decimal && is_valid_decimal <= SIGRTMAX) {
> +    *pnum = is_valid_decimal; 
> +    return 0; 
> +  }
> +
> +  /* If str is in RT signal range, get number of of RT signal, save it as an 
> +   * integer. The Issue 8 standard requires */
                                              ^^^
                                              Looks truncated?

> +  if (strncmp(str, "RTMIN+", SPACES_TO_N) == 0) {
> +    j = atoi(&str[SPACES_TO_N]);

Oops... strncmp returns 0, but you didn't check if the next char is
actually a non-NUL char, or even a digit.  Only digits are valid, so
atoi should be replaced with strtoul with an extra check for endptr.

> +    /* If number is valid, save it in pnum. */
> +    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)) {
                                               ^^^
                                               spaces

We have a special problem here.  i686 Cygwin only supports a single RT
signal.  For historical reasons, don't ask.

I. e., SIGRTMIN == SIGRTMAX.  This special case should be checked here,
to make sure the code works with this very special, very non-POSIX
behaviour.  I think the easiest way to handle that is to skip the
entire "RTMIN+"/"RTMAX-" code if SIGRTMIN == SIGRTMAX.  There just is
no such valid value in this case.

> +      *pnum = (SIGRTMIN + j);
> +      return 0;  
> +    }
> +    return -1; 
> +  }
> +  
> +  /* If str is in RT signal range, get number of of RT signal, save it as an 
> +   * integer. */
> +  if (strncmp(str, "RTMAX-", SPACES_TO_N) == 0) {
> +    j = atoi(&str[SPACES_TO_N]);

Ditto.

> +    /* If number is valid, save it in pnum. */
> +    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)) {
                                               ^^^
                                               spaces


Thanks,
Corinna