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

[Christian Brauner <christian@brauner.io>]

On Tue, May 29, 2018 at 07:24:26AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian@brauner.io> 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@brauner.io>
> > ---
> > 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,