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

[Corinna Vinschen <vinschen@redhat.com>]

Hi Matt,

thanks for this v2.

On Jul 24 10:37, 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.
> Added Makefile.am entries to build the new file.
> ---
> [...]
> +#if __GNU_VISIBLE

I think this needs discussion.  The sig2str/str2sig API has not been
provided yet by GLibC.  Using __GNU_VISIBLE in this context looks wrong.
What we need, in fact, is a __POSIX_VISIBLE guard, but here's the
problem: As far as I can see, the Issue 8 draft does not yet define a
version number.

If anybody has better information or a good idea how to guard this new
API in the meantime, I'm all ears.

>  
>  include $(srcdir)/../../Makefile.shared
>  
> -CHEWOUT_FILES = psignal.def raise.def signal.def
> +CHEWOUT_FILES = psignal.def raise.def signal.def sig2str.def

If you add this, you also have to provide a header allowing to create
a man page.  See, for instance, newlib/signal/raise.c.

> +int
> +sig2str(int signum, char *str)
> [...]
> +  /* If signum is not a recognized signal number, store this message in str. */
> +  sprintf(str, "Unknown signal"); 

Just drop this sprintf, as I just wrote in my other mail.  There's no
expectation that the incoming buffer gets changed if the function
returns with error.

> +  return -1; 
> +}
> +
> +int
> +str2sig(const char *restrict str, int *restrict pnum)
> +{
> +  unsigned long j = 0; 
> +  char *endp; 
> +  const sig_name_and_num *sptr; 
> +  int is_valid_decimal;
> +  is_valid_decimal = atoi(str);

What if the input is "12xyz"?  This, too, needs checking with strtoul.

The rest looks good to me.


Thanks,
Corinna