Re: [PATCH 25/30] bsd-user/signal.c: handle_pending_signal

[Peter Maydell]

On Sun, 9 Jan 2022 at 16:47, Warner Losh <imp@bsdimp.com> wrote:
>
> Handle a queued signal.
>
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
> Signed-off-by: Kyle Evans <kevans@freebsd.org>
> Signed-off-by: Warner Losh <imp@bsdimp.com>


> +static void handle_pending_signal(CPUArchState *cpu_env, int sig,
> +                                  struct emulated_sigtable *k)
> +{
> +    CPUState *cpu = env_cpu(cpu_env);
> +    TaskState *ts = cpu->opaque;
> +    struct qemu_sigqueue *q;
> +    struct target_sigaction *sa;
> +    int code;
> +    sigset_t set;
> +    abi_ulong handler;
> +    target_siginfo_t tinfo;
> +    target_sigset_t target_old_set;
> +
> +    trace_user_handle_signal(cpu_env, sig);
> +
> +    /* Dequeue signal. */
> +    q = k->first;
> +    k->first = q->next;
> +    if (!k->first) {
> +        k->pending = 0;
> +    }

(The dequeue simplifies if you follow linux-user's way of handling
"queueing" a signal.)

> +
> +    sig = gdb_handlesig(cpu, sig);
> +    if (!sig) {
> +        sa = NULL;
> +        handler = TARGET_SIG_IGN;
> +    } else {
> +        sa = &sigact_table[sig - 1];
> +        handler = sa->_sa_handler;
> +    }
> +
> +    if (do_strace) {
> +        print_taken_signal(sig, &q->info);
> +    }
> +
> +    if (handler == TARGET_SIG_DFL) {
> +        /*
> +         * default handler : ignore some signal. The other are job
> +         * control or fatal.
> +         */
> +        if (TARGET_SIGTSTP == sig || TARGET_SIGTTIN == sig ||
> +                TARGET_SIGTTOU == sig) {
> +            kill(getpid(), SIGSTOP);
> +        } else if (TARGET_SIGCHLD != sig && TARGET_SIGURG != sig &&
> +            TARGET_SIGINFO != sig &&
> +            TARGET_SIGWINCH != sig && TARGET_SIGCONT != sig) {
> +            force_sig(sig);
> +        }
> +    } else if (TARGET_SIG_IGN == handler) {

Avoid the yoda-conditionals, please.

> +        /* ignore sig */
> +    } else if (TARGET_SIG_ERR == handler) {
> +        force_sig(sig);

Note that if you follow linux-user and my suggestion on
patch 21, these force_sig() calls become calls to
dump_core_and_abort(), unlike the one in setup_frame(),
which should be a "queue this signal". (The difference is
that here we know the process is definitely going to die,
because it has no valid handler for a fatal signal. In
setup_frame() the process might be able to continue, if it
has a signal handler for the SIGILL or SIGSEGV or whatever.)

> +    } else {
> +        /* compute the blocked signals during the handler execution */
> +        sigset_t *blocked_set;
> +
> +        target_to_host_sigset(&set, &sa->sa_mask);
> +        /*
> +         * SA_NODEFER indicates that the current signal should not be
> +         * blocked during the handler.
> +         */
> +        if (!(sa->sa_flags & TARGET_SA_NODEFER)) {
> +            sigaddset(&set, target_to_host_signal(sig));
> +        }
> +
> +        /*
> +         * Save the previous blocked signal state to restore it at the
> +         * end of the signal execution (see do_sigreturn).
> +         */
> +        host_to_target_sigset_internal(&target_old_set, &ts->signal_mask);
> +
> +        blocked_set = ts->in_sigsuspend ?
> +            &ts->sigsuspend_mask : &ts->signal_mask;
> +        qemu_sigorset(&ts->signal_mask, blocked_set, &set);
> +        ts->in_sigsuspend = false;
> +        sigprocmask(SIG_SETMASK, &ts->signal_mask, NULL);
> +
> +        /* XXX VM86 on x86 ??? */
> +
> +        code = q->info.si_code;
> +        /* prepare the stack frame of the virtual CPU */
> +        if (sa->sa_flags & TARGET_SA_SIGINFO) {
> +            tswap_siginfo(&tinfo, &q->info);

Oh, you're doing the tswap_siginfo() here. If you really want to
do that, then the setup_frame() should be able to do a simple
structure-copy I think and doesn't need the logic to figure out
which union fields are relevant. But putting the tswap_siginfo()
inside setup_frame() would match where linux-user does it.

> +            setup_frame(sig, code, sa, &target_old_set, &tinfo, cpu_env);
> +        } else {
> +            setup_frame(sig, code, sa, &target_old_set, NULL, cpu_env);
> +        }
> +        if (sa->sa_flags & TARGET_SA_RESETHAND) {
> +            sa->_sa_handler = TARGET_SIG_DFL;
> +        }
> +    }
> +    if (q != &k->info) {
> +        free_sigqueue(cpu_env, q);
> +    }
> +}
> +
>  void process_pending_signals(CPUArchState *cpu_env)
>  {
>  }
> --
> 2.33.1

thanks
-- PMM