Re: [PATCH v2 3/4] linux-user: fix TARGET_NSIG and _NSIG uses

[Peter Maydell]

On Tue, 11 Feb 2020 at 16:59, Laurent Vivier <address@hidden> wrote:
>
> Le 11/02/2020 à 17:47, Peter Maydell a écrit :
> > On Tue, 4 Feb 2020 at 17:11, Laurent Vivier <address@hidden> wrote:
> >>
> >> Valid signal numbers are between 1 (SIGHUP) and SIGRTMAX.
> >>
> >> System includes define _NSIG to SIGRTMAX + 1, but
> >> QEMU (like kernel) defines TARGET_NSIG to TARGET_SIGRTMAX.
> >>
> >> Fix all the checks involving the signal range.
> >>
> >> Signed-off-by: Laurent Vivier <address@hidden>
> >> ---
> >>
> >> Notes:
> >>     v2: replace i, j by target_sig, host_sig
> >>
> >>  linux-user/signal.c | 52 ++++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 37 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/linux-user/signal.c b/linux-user/signal.c
> >> index 246315571c09..c1e664f97a7c 100644
> >> --- a/linux-user/signal.c
> >> +++ b/linux-user/signal.c
> >> @@ -30,6 +30,15 @@ static struct target_sigaction 
> >> sigact_table[TARGET_NSIG];
> >
> > Optional follow-on patch: make sigact_table[] also size
> > TARGET_NSIG + 1, for consistency with target_to_host_signal_table[],
> > and remove all the "- 1"s when we index into it.
> >
>
> OK,
>
> >> @@ -492,10 +514,10 @@ static void signal_table_init(void)
> >>          if (host_to_target_signal_table[host_sig] == 0) {
> >>              host_to_target_signal_table[host_sig] = host_sig;
> >>          }
> >> -    }
> >> -    for (host_sig = 1; host_sig < _NSIG; host_sig++) {
> >>          target_sig = host_to_target_signal_table[host_sig];
> >> -        target_to_host_signal_table[target_sig] = host_sig;
> >> +        if (target_sig <= TARGET_NSIG) {
> >> +            target_to_host_signal_table[target_sig] = host_sig;
> >> +        }
> >
> > Why does this hunk apparently delete the for() line ?
>
> It effectively deletes the for() line because I merge the two "for
> (host_sig = 1; host_sig < _NSIG; host_sig++)" loops into one.

Oh, I see, I missed the closing brace being deleted.

> > Why do we need the if() -- surely there should never be any
> > entries in host_to_target_signal_table[] that aren't
> > valid target signal numbers ?
> >
>
> we have above the "host_to_target_signal_table[host_sig] = host_sig;"
> and host_sig can be greater than TARGET_NSIG.
>
> Setting like this allows to ignore them later in the target as we can
> compare them to TARGET_NSIG. This mapping 1:1 in the default case is the
> original behaviour.

I guess so (I was sort of expecting us to do the filter on
"is this valid" when we filled the array, rather than having
to do it every time we used the entries, but this works).

Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM