Re: [PATCH v1 04/20] signal: add copy_pending() helper

From: Christian Brauner
Date: Tue May 29 2018 - 08:42:12 EST


On Tue, May 29, 2018 at 07:24:26AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian@xxxxxxxxxx> writes:
>
> > Instead of using a goto for this let's add a simple helper copy_pending()
> > which can be called in both places.
>
> Ick no. As far as I can see this just confuses the logic of the
> collect_signal function.
>
> Instead of having two cases with an optional
> "sigdelset(&list->signal, sig)" if the signal is no longer in the queue,
> you are moving the core work of collect_signal into another function.
>
> At the very least this is going to make maintenance more difficult
> as now the work of this function is split into two functions.

I do disagree here tbh. The goto jump into it the if part of an if-else
seems pretty nasty.
I also don't know why this should be confusing the logic. There's a
single function that is called in two places and it is declared directly
atop it's only caller. Additionally, recognizing a single name of a
function as being the same in two places is way easier then recognizing
that a multi-line pattern is the same in two places.

Christian

>
> It most definitely would have made the bug fix to add resched_timer more
> difficult.
>
> Eric
>
> > Signed-off-by: Christian Brauner <christian@xxxxxxxxxx>
> > ---
> > v0->v1:
> > * patch unchanged
> > ---
> > kernel/signal.c | 54 +++++++++++++++++++++++++++----------------------
> > 1 file changed, 30 insertions(+), 24 deletions(-)
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index bc750fb4ddcc..baae137455eb 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -515,6 +515,19 @@ int unhandled_signal(struct task_struct *tsk, int sig)
> > return !tsk->ptrace;
> > }
> >
> > +static void copy_pending(siginfo_t *info, struct sigqueue *first,
> > + bool *resched_timer)
> > +{
> > + list_del_init(&first->list);
> > + copy_siginfo(info, &first->info);
> > +
> > + *resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
> > + (info->si_code == SI_TIMER) &&
> > + (info->si_sys_private);
> > +
> > + __sigqueue_free(first);
> > +}
> > +
> > static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
> > bool *resched_timer)
> > {
> > @@ -526,8 +539,10 @@ static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
> > */
> > list_for_each_entry(q, &list->list, list) {
> > if (q->info.si_signo == sig) {
> > - if (first)
> > - goto still_pending;
> > + if (first) {
> > + copy_pending(info, first, resched_timer);
> > + return;
> > + }
> > first = q;
> > }
> > }
> > @@ -535,29 +550,20 @@ static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
> > sigdelset(&list->signal, sig);
> >
> > if (first) {
> > -still_pending:
> > - list_del_init(&first->list);
> > - copy_siginfo(info, &first->info);
> > -
> > - *resched_timer =
> > - (first->flags & SIGQUEUE_PREALLOC) &&
> > - (info->si_code == SI_TIMER) &&
> > - (info->si_sys_private);
> > -
> > - __sigqueue_free(first);
> > - } else {
> > - /*
> > - * Ok, it wasn't in the queue. This must be
> > - * a fast-pathed signal or we must have been
> > - * out of queue space. So zero out the info.
> > - */
> > - clear_siginfo(info);
> > - info->si_signo = sig;
> > - info->si_errno = 0;
> > - info->si_code = SI_USER;
> > - info->si_pid = 0;
> > - info->si_uid = 0;
> > + copy_pending(info, first, resched_timer);
> > + return;
> > }
> > +
> > + /*
> > + * Ok, it wasn't in the queue. This must be a fast-pathed signal or we
> > + * must have been out of queue space. So zero out the info.
> > + */
> > + clear_siginfo(info);
> > + info->si_signo = sig;
> > + info->si_errno = 0;
> > + info->si_code = SI_USER;
> > + info->si_pid = 0;
> > + info->si_uid = 0;
> > }
> >
> > static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,