Re: [PATCH] signal: Fix the error return of kill -1

["Eric W. Biederman" <ebiederm@xmission.com>]

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