Messages in this thread | | | From | "Eric W. Biederman" <> | Date | Wed, 16 Aug 2023 21:33:35 -0500 | Subject | Re: [PATCH] signal: Fix the error return of kill -1 |
| |
Oleg Nesterov <oleg@redhat.com> writes:
> On 08/16, Eric W. Biederman wrote: >> >> Oleg Nesterov <oleg@redhat.com> writes: >> >> > On 08/15, David Laight wrote: >> >> >> >> or maybe even: >> >> } else { >> >> struct task_struct * p; >> >> int err; >> >> ret = -ESRCH; >> >> >> >> for_each_process(p) { >> >> if (task_pid_vnr(p) > 1 && >> >> !same_thread_group(p, current)) { >> >> err = group_send_sig_info(sig, info, p, >> >> PIDTYPE_MAX); >> >> if (ret) >> >> ret = err; >> > >> > Hmm, indeed ;) >> > >> > and "err" can be declared inside the loop. >> >> We can't remove the success case, from my posted patch. >> >> A signal is considered as successfully delivered if at least >> one process receives it. > > Yes. > > Initially ret = -ESRCH. > > Once group_send_sig_info() succeeds at least once (returns zero) > ret becomes 0. > > After that > > if (ret) > ret = err; > > has no effect. > > So if a signal is successfully delivered at least once the code > above returns zero.
Point.
We should be consistent and ensure __kill_pgrp_info uses the same code pattern, otherwise it will be difficult to see they use the same logic.
Does "if (ret) ret = err;" generate better code than "success |= !err"?
I think for both patterns the reader of the code is going to have to stop and think about what is going on to understand the logic.
We should probably do something like:
/* 0 for success or the last error */ if (ret) ret = err;
I am somewhat partial to keeping the variable "success" simply because while it's computation is clever it's use in generating the result is not, so it should be more comprehensible code. Plus the variable success seems not to need a comment just a minute to stare at the code and confirm it works.
Eric
| |