Christian Brauner <[email protected]> writes:
> 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.
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
