[PATCH 1/2] signals: fold sig_ignored() into handle_stop_signal() - Kernel

This is a discussion on [PATCH 1/2] signals: fold sig_ignored() into handle_stop_signal() - Kernel ; Rename handle_stop_signal() to prepare_signal(), make it return a boolean, and move the callsites of sig_ignored() into it. No functional changes for now. But it would be nice to factor out the "should we drop this signal" checks as much as ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH 1/2] signals: fold sig_ignored() into handle_stop_signal()

  1. [PATCH 1/2] signals: fold sig_ignored() into handle_stop_signal()

    Rename handle_stop_signal() to prepare_signal(), make it return a boolean, and
    move the callsites of sig_ignored() into it.

    No functional changes for now. But it would be nice to factor out the "should
    we drop this signal" checks as much as possible, before we try to fix the bugs
    with the sub-namespace init's signals (actually the global /sbin/init has some
    problems with signals too).

    Signed-off-by: Oleg Nesterov

    --- 25/kernel/signal.c~1_HSS_PREPARE_SIGNAL 2008-03-12 11:42:31.000000000 +0300
    +++ 25/kernel/signal.c 2008-03-12 12:58:39.000000000 +0300
    @@ -564,18 +564,16 @@ static void do_notify_parent_cldstop(str
    * actual continuing for SIGCONT, but not the actual stopping for stop
    * signals. The process stop is done as a signal action for SIG_DFL.
    */
    -static void handle_stop_signal(int sig, struct task_struct *p)
    +static int prepare_signal(int sig, struct task_struct *p)
    {
    struct signal_struct *signal = p->signal;
    struct task_struct *t;

    - if (signal->flags & SIGNAL_GROUP_EXIT)
    + if (unlikely(signal->flags & SIGNAL_GROUP_EXIT)) {
    /*
    - * The process is in the middle of dying already.
    + * The process is in the middle of dying, nothing to do.
    */
    - return;
    -
    - if (sig_kernel_stop(sig)) {
    + } else if (sig_kernel_stop(sig)) {
    /*
    * This is a stop signal. Remove SIGCONT from all queues.
    */
    @@ -644,6 +642,8 @@ static void handle_stop_signal(int sig,
    signal->flags &= ~SIGNAL_STOP_DEQUEUED;
    }
    }
    +
    + return !sig_ignored(p, sig);
    }

    /*
    @@ -753,7 +753,8 @@ static int send_signal(int sig, struct s
    struct sigqueue *q;

    assert_spin_locked(&t->sighand->siglock);
    - handle_stop_signal(sig, t);
    + if (!prepare_signal(sig, t))
    + return 0;

    pending = group ? &t->signal->shared_pending : &t->pending;
    /*
    @@ -761,7 +762,7 @@ static int send_signal(int sig, struct s
    * exactly one non-rt signal, so that we can get more
    * detailed information about the cause of the signal.
    */
    - if (sig_ignored(t, sig) || legacy_queue(pending, sig))
    + if (legacy_queue(pending, sig))
    return 0;

    /*
    @@ -1247,10 +1248,8 @@ int send_sigqueue(struct sigqueue *q, st
    if (!likely(lock_task_sighand(t, &flags)))
    goto ret;

    - handle_stop_signal(sig, t);
    -
    - ret = 1;
    - if (sig_ignored(t, sig))
    + ret = 1; /* the signal is ignored */
    + if (!prepare_signal(sig, t))
    goto out;

    ret = 0;

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

  2. Re: [PATCH 1/2] signals: fold sig_ignored() into handle_stop_signal()

    This one looks fine to me. I would like the comment above the function to
    be updated to describe its new purpose, and to document its return value's
    meaning.

    Also, I'm not sure if there is a kernel code formatting convention that
    particularly excludes an empty block ({}, equiv to containing just
    comments. But I don't recall seeing that style used in the kernel.
    (I don't mind it personally for this case given what the obvious
    alternatives would look like.)


    Thanks,
    Roland

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

  3. Re: [PATCH 1/2] signals: fold sig_ignored() into handle_stop_signal()

    On 03/12, Roland McGrath wrote:
    >
    > This one looks fine to me. I would like the comment above the function to
    > be updated to describe its new purpose, and to document its return value's
    > meaning.


    Will do.

    > Also, I'm not sure if there is a kernel code formatting convention that
    > particularly excludes an empty block ({}, equiv to containing just
    > comments. But I don't recall seeing that style used in the kernel.
    > (I don't mind it personally for this case given what the obvious
    > alternatives would look like.)


    Yes, this looks a bit unusual... but I can't see how it is possible to
    make it simpler (without goto's or duplication the code).

    > > A couple of small comments about how CLD_CONTINUED notification works.

    >
    > I would make the get_signal_to_deliver comment say a
    > little more. In particular, it's kind of nonobvious that though this
    > happens at the beginning of the function, the importance of its placement
    > is really that it will always be run (via the relock: loop) just after any
    > wake-up from having been in TASK_STOPPED.


    Will do, thanks.

    Oleg.

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

+ Reply to Thread