manual: don't ignore SIGCHLD when calling waitpid

  From: emersion <contact@emersion.fr>

If SIGCHLD is ignore, child process information is discarded as soon as they
exit, making it impossible to retrieve their status with waitpid.
---
 ChangeLog       | 4 ++++
 manual/job.texi | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)
  

Hi,

Could anyone review this? I can't find a maintainer to Cc on the wiki.

Thanks,

Simon Ser

On Thursday, January 17, 2019 4:35 PM, Simon Ser <contact@emersion.fr> wrote:
> From: emersion <contact@emersion.fr>
>
> If SIGCHLD is ignore, child process information is discarded as soon as they
> exit, making it impossible to retrieve their status with waitpid.
> ---
>  ChangeLog       | 4 ++++
>  manual/job.texi | 2 --
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 4c34d45a..661d2749 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-01-17  Simon Ser  <contact@emersion.fr>
> +
> +	* manual/job.texi: don't ignore SIGCHLD when calling waitpid
> +
>  2019-01-16  Paul A. Clarke  <pc@us.ibm.com>
>
>  	* sysdeps/powerpc/powerpc64/multiarch/strncmp.c: Fix #ifdef.
> diff --git a/manual/job.texi b/manual/job.texi
> index 05a42ea8..0fc7f51a 100644
> --- a/manual/job.texi
> +++ b/manual/job.texi
> @@ -414,7 +414,6 @@ init_shell ()
>        signal (SIGTSTP, SIG_IGN);
>        signal (SIGTTIN, SIG_IGN);
>        signal (SIGTTOU, SIG_IGN);
> -      signal (SIGCHLD, SIG_IGN);
>
>        /* @r{Put ourselves in our own process group.}  */
>        shell_pgid = getpid ();
> @@ -530,7 +529,6 @@ launch_process (process *p, pid_t pgid,
>        signal (SIGTSTP, SIG_DFL);
>        signal (SIGTTIN, SIG_DFL);
>        signal (SIGTTOU, SIG_DFL);
> -      signal (SIGCHLD, SIG_DFL);
>      @}
>
>    /* @r{Set the standard input/output channels of the new process.}  */
> --
> 2.20.1
  

My understanding of the example is for interactive shells (shell_is_interactive),
the idea is only to get child information to the explicit launched processes done
by launch_process, so ignoring and reseting SIGCHLD seems ok.

On 17/04/2019 15:34, Simon Ser wrote:
> Hi,
> 
> Could anyone review this? I can't find a maintainer to Cc on the wiki.
> 
> Thanks,
> 
> Simon Ser
> 
> On Thursday, January 17, 2019 4:35 PM, Simon Ser <contact@emersion.fr> wrote:
>> From: emersion <contact@emersion.fr>
>>
>> If SIGCHLD is ignore, child process information is discarded as soon as they
>> exit, making it impossible to retrieve their status with waitpid.
>> ---
>>  ChangeLog       | 4 ++++
>>  manual/job.texi | 2 --
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 4c34d45a..661d2749 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,7 @@
>> +2019-01-17  Simon Ser  <contact@emersion.fr>
>> +
>> +	* manual/job.texi: don't ignore SIGCHLD when calling waitpid
>> +
>>  2019-01-16  Paul A. Clarke  <pc@us.ibm.com>
>>
>>  	* sysdeps/powerpc/powerpc64/multiarch/strncmp.c: Fix #ifdef.
>> diff --git a/manual/job.texi b/manual/job.texi
>> index 05a42ea8..0fc7f51a 100644
>> --- a/manual/job.texi
>> +++ b/manual/job.texi
>> @@ -414,7 +414,6 @@ init_shell ()
>>        signal (SIGTSTP, SIG_IGN);
>>        signal (SIGTTIN, SIG_IGN);
>>        signal (SIGTTOU, SIG_IGN);
>> -      signal (SIGCHLD, SIG_IGN);
>>
>>        /* @r{Put ourselves in our own process group.}  */
>>        shell_pgid = getpid ();
>> @@ -530,7 +529,6 @@ launch_process (process *p, pid_t pgid,
>>        signal (SIGTSTP, SIG_DFL);
>>        signal (SIGTTIN, SIG_DFL);
>>        signal (SIGTTOU, SIG_DFL);
>> -      signal (SIGCHLD, SIG_DFL);
>>      @}
>>
>>    /* @r{Set the standard input/output channels of the new process.}  */
>> --
>> 2.20.1
  

On 4/17/19 12:05 PM, Adhemerval Zanella wrote:
> My understanding of the example is for interactive shells (shell_is_interactive),
> the idea is only to get child information to the explicit launched processes done
> by launch_process, so ignoring and reseting SIGCHLD seems ok.

Sure, but the problem is that launch_process is used only in the child,
which means the parent shell always ignores SIGCHLD, so if the parent
calls do_job_notification it won't get any info about children. Perhaps
this could be fixed by documenting do_job_notification as requiring that
SIGCHLD be reenabled before calling it, but that's pretty confusing.

Also the proposed change to the manual is not enough, as there's other
text saying that the sample shell program ignores SIGCHLD. Presumably
this is to avoid races in the parent shell. But if SIGCHLD causes races
in the parent, then the parent typically should block SIGCHLD instead of
ignoring it and similarly for the other signals - otherwise the fact
that the signals arrived will be lost. So the whole example needs to be
rethought.
  

On 17/04/2019 16:44, Paul Eggert wrote:
> On 4/17/19 12:05 PM, Adhemerval Zanella wrote:
>> My understanding of the example is for interactive shells (shell_is_interactive),
>> the idea is only to get child information to the explicit launched processes done
>> by launch_process, so ignoring and reseting SIGCHLD seems ok.
> 
> Sure, but the problem is that launch_process is used only in the child,
> which means the parent shell always ignores SIGCHLD, so if the parent
> calls do_job_notification it won't get any info about children. Perhaps
> this could be fixed by documenting do_job_notification as requiring that
> SIGCHLD be reenabled before calling it, but that's pretty confusing.

Right, I missed this part indeed.

> 
> Also the proposed change to the manual is not enough, as there's other
> text saying that the sample shell program ignores SIGCHLD. Presumably
> this is to avoid races in the parent shell. But if SIGCHLD causes races
> in the parent, then the parent typically should block SIGCHLD instead of
> ignoring it and similarly for the other signals - otherwise the fact
> that the signals arrived will be lost. So the whole example needs to be
> rethought.
> 

Agreed.