Re: [PATCH 2/2] pasta: propagate exit code from child command

[Paul Holzinger <pholzing@redhat.com>]

Thanks for the feedback, I will send a v2 with the comments fixed.

On 08/02/2023 19:14, Stefano Brivio wrote:
> On Wed,  8 Feb 2023 16:54:41 +0100
> Paul Holzinger <pholzing@redhat.com> wrote:
>
>> Exits codes are very useful for scripts, when the pasta child execvp()
>> call fails with ENOENT that parent should also exit with > 0. In short
>> the parent should always exit with the code from the child to make it
>> useful in scripts.
>>
>> It is easy to test with: `pasta -- bash -c "exit 3"; echo $?`
>>
>> Signed-off-by: Paul Holzinger <pholzing@redhat.com>
>> ---
>>   pasta.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/pasta.c b/pasta.c
>> index 3f6477c..4b18d7e 100644
>> --- a/pasta.c
>> +++ b/pasta.c
>> @@ -64,9 +64,14 @@ void pasta_child_handler(int signal)
>>   
>>   	if (pasta_child_pid &&
>>   	    !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
>> -		if (infop.si_pid == pasta_child_pid)
>> -			exit(EXIT_SUCCESS);
>> -			/* Nothing to do, detached PID namespace going away */
>> +		if (infop.si_pid == pasta_child_pid) {
>> +			if (infop.si_code == CLD_EXITED)
>> +				exit(infop.si_status);
>> +
>> +			/* else killed by signal, si_status == SIGNUM in this case */
> It took me a while to figure out that SIGNUM is not a constant I wasn't
> aware of, just the signal number -- I guess you meant 'signum' from
> sigaction() or suchlike?
>
> Maybe you could go with:
>
> 			/* If killed by a signal, si_status is its number */
>
>> +			exit(infop.si_status + 128);
> This is a bit arbitrary -- returning n + 128 is what Bash, zsh, and a
> few other shells do, and probably a reasonable convention given that
> POSIX says we need to return a value greater than 128. Still I would
> mention it:
>
> 			/* Arbitrary: return signal number + 128 */
>
> or:
>
> 			/* If killed by a signal, si_status is the number.
> 			 * Follow common shell convention of returning it + 128.
> 			 */

The second suggestion sounds good to me.

>> +		}
>> +		/* Nothing to do, detached PID namespace going away */
> This should be moved back into the conditional block (swapping the two
> lines above, or moving this comment at the beginning of the block).
Whoops, this was by accident.
>
> If si_pid is not the child, we need to do other stuff (that is, reap
> clones).
>
>>   	}
>>   
>>   	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);