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

[ebiederm@xmission.com (Eric W. Biederman)]

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.

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,