On Tue, May 29, 2018 at 07:24:26AM -0500, Eric W. Biederman wrote: > Christian Brauner <[email protected]> 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 <[email protected]> > > --- > > 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,
