Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

[Linus Torvalds]

On Mon, Apr 11, 2011 at 10:21 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> I am not sure this is bug, but at least this looks strange imho. T1 should
> not sleep forever, there is a signal which should wake it up.

Hmm. I worry about the overhead of this, and I'm not 100% convinced we need it.

But my _biggest_ objection to the series is a purely technical one:

> --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200
> +++ sigprocmask/include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200
> @@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set,
>        }
>
>        spin_lock_irq(&tsk->sighand->siglock);
> +       if (signal_pending(tsk) && !thread_group_empty(tsk)) {
> +               sigset_t not_newblocked;
> +               signorsets(&not_newblocked, &current->blocked, &newset);
> +               retarget_shared_pending(tsk, &not_newblocked);
> +       }
>        tsk->blocked = newset;
>        recalc_sigpending();
>        spin_unlock_irq(&tsk->sighand->siglock);

I absolutely detest how you made "sigprocmask()" the main interface to
do this all, and then add new callers.

It's a horrid interface with that crazy "how" argument, and comes out
of the user-space system call interface. If we make kernel users do
this, especially critical ones like the signal handling code, please
just extract out just the actual "set new signal mask" part.

So please just introduce a "sig_set_blocked()" or something, without
the crazy "switch (how)" crud, and make sigprocmask() and everybody
else use _that_ instead.

That would make me much happier about the patch series, I suspect.

                     Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/