Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 29 May 2018 08:44:22 -0500 | Subject | Re: [PATCH v1 04/20] signal: add copy_pending() helper |
| |
Christian Brauner <christian@brauner.io> writes:
> 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.
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
| |