[PATCH 2/2] libc: Edited implementations for sig2str/str2sig

[Joel Sherrill]

This should have been folded into the other patch so we reviewed a
final version.

On Wed, Jul 7, 2021 at 5:47 AM Matt Joyce <mfjoyce2004 at gmail.com> wrote:
>
> Added features to function implementations to conform with POSIX standard.
> ---
>  newlib/libc/signal/sig2str.c | 96 ++++++++++++++++++++++++++++++------
>  1 file changed, 81 insertions(+), 15 deletions(-)
>
> diff --git a/newlib/libc/signal/sig2str.c b/newlib/libc/signal/sig2str.c
> index a07fa2a38..152d10cf7 100644
> --- a/newlib/libc/signal/sig2str.c
> +++ b/newlib/libc/signal/sig2str.c
> @@ -1,9 +1,14 @@
>  /* Placeholder  */
> -//#define __GNU_VISIBLE // defining it to have access to SIG2STR_MAX
>
> +/* Defining _GNU_SOURCE to have access to SIG2STR_MAX in signal.h. */
> +#define _GNU_SOURCE
>  #include <signal.h>
>  #include <string.h>
>  #include <stdio.h>
> +#include <stdlib.h>
> +
> +#define SPACES_TO_N 6
> +#define NUM_OF_SIGS (sizeof(sig_array) / sizeof(sig_name_and_num))
>
>  typedef struct sig_name_and_num {
>    const char *sig_name;
> @@ -101,12 +106,12 @@ static sig_name_and_num sig_array[] = {
>      #ifdef SIGUSR2
>      { "USR2", SIGUSR2 },
>      #endif
> -    // #ifdef SIGRTMIN
> -    // { "RTMIN", SIGRTMIN },
> -    // #endif
> -    // #ifdef SIGRTMAX
> -    // { "RTMAX", SIGRTMAX },
> -    // #endif
> +    #ifdef SIGRTMIN
> +    { "RTMIN", SIGRTMIN },
> +    #endif
> +    #ifdef SIGRTMAX
> +    { "RTMAX", SIGRTMAX },
> +    #endif

Why is this in here? You won't get it as input. RT signals get processed
special AFAIK looking at other implementations.

>      #ifdef SIGPWR
>      { "PWR", SIGPWR },
>      #endif
> @@ -127,25 +132,86 @@ static sig_name_and_num sig_array[] = {
>      #endif
>  };
>
> -#define NUM_OF_SIGS (sizeof(sig_array) / sizeof(sig_name_and_num))
> -
>  int
>  sig2str(int signum, char *str)
>  {
> +  /* Real Time Signals, lower half */

I just don't understand the upper or lower half. It is RTSIGnn and the
nn is what you are computing.

> +  if ((SIGRTMIN + 1) <= signum && signum <= (SIGRTMIN + SIGRTMAX) / 2){

I think this should be written >= and <= to indicate a range. I don't
think this idiom is used frequently.

> +    sprintf(str, "RTMIN+%d", (signum-SIGRTMIN));
> +    return 0;
> +  }
>
> -  for (sig_name_and_num *i = sig_array; i < &sig_array[NUM_OF_SIGS]; i++){
> -    if (i->sig_num == signum){
> +  /* Real Time Signals, upper half */
> +  else if ((((SIGRTMIN +SIGRTMAX)/ 2) + 1) <= signum && signum <= \

Why the line break?

Why the splitting of the range? I think RT signals are guaranteed to
be consecutive
between MIN and MAX so a simple (signum >= SIGRTMIN && signum <= SIGRTMAX)
should be sufficient.


> +  (SIGRTMAX - 1)){
> +    sprintf(str, "RTMAX-%d", (SIGRTMAX-signum));
> +    return 0;
> +  }

Spacing on ){

> +
> +  /* All others, including SIGRTMIN / SIGRTMAX */
> +  else{

You don't need an else if the RT signals were processed above. Something like:

if (signum >= SIGRTMIN && signum <= SIGRTMAX) {
  /* process it */
  return 0;
}

and then fall into the other loop.

> +    for (sig_name_and_num * i = sig_array; i < &sig_array[NUM_OF_SIGS]; i++){
> +      if (i->sig_num == signum){
>          strcpy(str, i->sig_name);
>          return 0;
> -    }
> +      }
> +    }
> +    sprintf(str, "Unknown signal %d", signum);
> +    return -1;
>    }
> -  sprintf(str, "Unknown signal %d", signum);
> -  return -1;
>  }
>
>  int
> -str2sig(const char *__restrict str, int *__restrict pnum)
> +str2sig(const char *restrict str, int *restrict pnum)

Why not use the __restrict like the rest of newlib?

>  {
> +  int j = 0;
> +  char dest[SIG2STR_MAX];
> +  int 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;
> +  }

Is this if condition right?

And "is_valid_decimal" seems an odd name. It is converting to a number.
If invalid, deal with it. If valid, it needs to be in range.

> +
> +  /* If str is in RT signal range, get number of of RT signal, save it as an
> +   * integer. */

of of

> +  if (strncmp(str, "RTMIN+", SPACES_TO_N) == 0){
> +    for(int i = SPACES_TO_N; str[i] != '\0'; i++){
> +      dest[j] = str[i];
> +      j++;
> +    }
> +    dest[j] = '\0';
> +    j = atoi(dest);
> +
> +    /* If number is valid, save it in pnum. */
> +    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)){
> +      *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){
> +    for(int i = SPACES_TO_N; str[i] != '\0'; i++){
> +      dest[j] = str[i];
> +      j++;
> +    }
> +    dest[j] = '\0';
> +    j = atoi(dest);
> +
> +    /* If number is valid, save it in pnum. */
> +    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)){
> +      *pnum = (SIGRTMAX - j);
> +      return 0;
> +    }
> +    return -1;
> +  }
> +
> +  /*If str is a valid signal name, save its corresponding number in pnum. */
>    for (sig_name_and_num *i = sig_array; i < &sig_array[NUM_OF_SIGS]; i++){
>      if (strcmp(i->sig_name, str) == 0){
>          *pnum = i->sig_num;

Watch the formatting.

> --
> 2.31.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel