signal.c: Change printsignal to properly handle xlat styles

[shankarapailoor]

>btw, have you tried to invoke k_sigaction with an invalid signal number,
e.g. -1?  Does the output look good in verbose mode?

Ah good point. I didn't consider that. With my change verbose mode
prints -1 /* -1 */ which is wrong. Here is a patch which addresses
that along with your above comments.

On Fri, Dec 21, 2018 at 5:14 PM Dmitry V. Levin <ldv at altlinux.org> wrote:
>
> Hi Shankara,
>
> On Thu, Dec 20, 2018 at 05:32:02AM -0800, shankarapailoor wrote:
> > Hi!
> >
> > I noticed that strace doesn't properly decode the signum of sigaction
> > system calls under different xlat styles. Attached is a patch which
> > corrects this issue. I will follow this up with another patch to
> > change the decoding of sa_mask field to properly handle xlat styles.
> > Let me know what you think.
> >
> > Regards,
> > Shankara Pailoor
>
> > From 5d1d442be8765e12b955ffc3634c1a9e97849958 Mon Sep 17 00:00:00 2001
> > From: Shankara Pailoor <shankarapailoor at gmail.com>
> > Date: Thu, 20 Dec 2018 05:27:40 -0800
> > Subject: [PATCH v1] signal.c: Change printsignal to properly handle xlat styles
> >       * tests/printsignal.c: New test to make sure signum is printed
> >  properly under different xlat styles
> >       * tests/printsignal-Xabbrev.c: -Xabbrev case
> >       * tests/printsignal-Xraw.c: Likewise
> >       * tests/printsignal-Xverbose.c: Likewise
> >       * tests/pure_executables.list: Add new tests
> >       * tests/.gitignore: Likewise
>
> Please add an empty line after the first line of commit message,
> otherwise it becomes a single line of enormous length.
>
> > ---
> >  signal.c                     | 12 +++++++++
> >  tests/.gitignore             |  3 +++
> >  tests/gen_tests.in           |  3 +++
> >  tests/printsignal-Xabbrev.c  |  1 +
> >  tests/printsignal-Xraw.c     |  2 ++
> >  tests/printsignal-Xverbose.c |  2 ++
> >  tests/printsignal.c          | 48 ++++++++++++++++++++++++++++++++++++
> >  tests/pure_executables.list  |  3 +++
> >  8 files changed, 74 insertions(+)
> >  create mode 100644 tests/printsignal-Xabbrev.c
> >  create mode 100644 tests/printsignal-Xraw.c
> >  create mode 100644 tests/printsignal-Xverbose.c
> >  create mode 100644 tests/printsignal.c
> >
> > diff --git a/signal.c b/signal.c
> > index 17a3f79c..25002e60 100644
> > --- a/signal.c
> > +++ b/signal.c
> > @@ -228,7 +228,19 @@ sprint_old_sigmask_val(const char *const prefix, const unsigned long mask)
> >  void
> >  printsignal(int nr)
> >  {
> > +     if (xlat_verbose(xlat_verbosity) != XLAT_STYLE_ABBREV)
> > +             tprintf("%d", nr);
> > +
> > +     if (xlat_verbose(xlat_verbosity) == XLAT_STYLE_RAW)
> > +             return;
> > +
> > +     if (xlat_verbose(xlat_verbosity) == XLAT_STYLE_VERBOSE)
> > +             tprints(" /* ");
> > +
> >       tprints(signame(nr));
> > +
> > +     if (xlat_verbose(xlat_verbosity) == XLAT_STYLE_VERBOSE)
> > +             tprints(" */");
>
> Eugene in his branch has the following expressive statement for this case:
>
>         (xlat_verbose(xlat_verbosity) == XLAT_STYLE_VERBOSE
>                 ? tprints_comment : tprints)(signame(nr));
>
> > +#include <asm/unistd.h>
> > +
> > +#ifdef __NR_rt_sigaction
>
> I think you can safely assume that __NR_rt_sigaction is universally
> available.
>
> > +#include <assert.h>
>
> I don't see any use of assert here.
>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <signal.h>
> > +#include <stdio.h>
> > +
> > +static long
> > +k_sigaction(int signum, const struct sigaction *act,
> > +                     struct sigaction *oldact, size_t sigsetsize)
> > +{
> > +     return syscall(__NR_rt_sigaction, signum, act, oldact, sigsetsize);
> > +}
> > +
> > +int
> > +main(void)
> > +{
> > +     k_sigaction(SIGUSR2, NULL, NULL, sizeof(sigset_t));
> > +#if XLAT_RAW
> > +     printf("rt_sigaction(12, NULL, NULL, %lu)"
> > +             " = -1 EINVAL (Invalid argument)\n", sizeof(sigset_t));
> > +#elif XLAT_VERBOSE
> > +     printf("rt_sigaction(12 /* SIGUSR2 */, NULL, NULL, %lu)"
> > +             " = -1 EINVAL (Invalid argument)\n", sizeof(sigset_t));
> > +#else /* XLAT_ABBREV */
> > +     printf("rt_sigaction(SIGUSR2, NULL, NULL, %lu) ="
> > +             " -1 EINVAL (Invalid argument)\n", sizeof(sigset_t));
> > +#endif
>
> The value of SIGUSR2 differs across architectures, see linux/signalent.h
> and linux/*/signalent.h files.
>
> The only reason why this syscall fails with EINVAL is the size of sigset_t
> which in this case depends on libc.
>
> As in this test it doesn't matter whether the syscall succeeded or failed,
> you can use "errstr = sprintrc(rc)" idiom instead.
>
> btw, have you tried to invoke k_sigaction with an invalid signal number,
> e.g. -1?  Does the output look good in verbose mode?
>
>
> --
> ldv
> --
> Strace-devel mailing list
> Strace-devel at lists.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel



-- 
Regards,
Shankara Pailoor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v1-0001-signal.c-Change-printsignal-to-properly-handle-xl.patch
Type: text/x-patch
Size: 5198 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20181221/864a40f0/attachment.bin>