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

[Christian Brauner]

On Tue, May 29, 2018 at 08:44:22AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian@xxxxxxxxxx> writes:
> 
> > 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.
> 
> But there are not two places.  There is only one place.

Which is reachable from two different places and the goal was to avoid
having to jump into the second location with a goto in an if-else
construct.

> The logic might be cleaned up reorganizing the tests a little bit.
> Something like this perhaps.
> 
> 	/*
> 	 * Collect the siginfo appropriate to this signal.  Check if
> 	 * there is another siginfo for the same signal.
> 	*/
> 	list_for_each_entry(q, &list->list, list) {
> 		if (q->info.si_signo == sig) {
> 			if (first)
>                         	break;
> 			first = q;
> 		}
> 	}
> 
> 	/* Not still pending? */
> 	if (!first || (&q->list != &list->list))
>         	sigdelset(&list->signal, sig);
> 	if (first) {
> 		...
> 
> 
> The logic at a high level is:
> 	Is there another instance of this signal pending?
>              yes?  Then don't "sigdelset"
>         Do we have siginfo?
>            yes? return it.
>            no?  dummy up a siginfo.
> 
> Making that logic clearer would be nice.  Obscuring it with

I'm happy to change this in v2. But there's nothing obscure about
calling a helper function in two places while I can keep the definiton
of the helper function and the two places that it is called in on the
same 80x43 terminal.

Christian

> an extra function just obstructs maintenance.
> 
> Eric