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

[Matthew Joyce <mfjoyce2004@gmail.com>]

Hi Corinna,

Thank you again for the feedback! I'll make the changes as you
recommend. Two things to note:

1) Regarding the header guard, I'm also all ears! Joel was also
concerned about this but ultimately recommended __GNU_VISIBLE for now,
while there is still no version number. I'll stand by if there are any
alternative suggestions on this.

2) "EXIT": As a beginner, obviously please take what I say with
several grains of salt! My rationale for not including 0 / EXIT was
line 61853 where it states "if signum is equal to 0, the behavior is
unspecified", and the end of the RATIONALE section where it says "the
behavior in this case has been made unspecified." I'd think if we
added in EXIT / 0, we'd be going against those statements.

Thanks again!

Sincerely,

Matt


On Wed, Jul 28, 2021 at 8:40 PM Corinna Vinschen <vinschen@redhat.com> wrote:
>
> On Jul 28 11:11, Corinna Vinschen wrote:
> > 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.
>
> Additionally, there's a RATIONALE section in the draft, which describes
> how "historical versions of these functions translated a signum value 0
> to "EXIT" (and vice versa), so that they could be used by the shell for
> the trap utility."
>
> While this isn't going to become part of the standard, I wonder if we
> should follow suite, for maximum compatibility.
>
>
> Corinna
>