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

[Eric W. Biederman]

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.
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
an extra function just obstructs maintenance.

Eric