Messages in this thread | | | From | "Eric W. Biederman" <> | Date | Wed, 16 Aug 2023 23:37:37 -0500 | Subject | Re: [PATCH] signal: Fix the error return of kill -1 |
| |
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> 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 just looked at the assembly output and at least on x86 with cmov "if (ret) ret = err;" generates the better assembly even in the inner loop.
> 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; >
Even with that comment it feels awkward to me.
Does anyone have any idea how to make that idiom more obvious what is happening?
Eric
| |