From: Johannes Sixt <j6t@kdbg.org>
Date: Fri, 06 Nov 2015 05:51:27 GMT
Subject: Re: [PATCH 1/2] run-command: Remove set_nonblocking
Message-ID: <563C3FDF.9050304@kdbg.org>
In-Reply-To: <CAGZ79kZs8HQegNy=L5nGMUGJ98s-xgce1Ncrxpxe_ReaJXMjKw@mail.gmail.com>

Am 05.11.2015 um 23:20 schrieb Stefan Beller:
> On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>
>> diff --git a/run-command.c b/run-command.c
>> index 51d078c..3e42299 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,
>>          for (i = 0; i < n; i++) {
>>                  strbuf_init(&pp->children[i].err, 0);
>>                  child_process_init(&pp->children[i].process);
>> -               pp->pfd[i].events = POLLIN;
>> +               pp->pfd[i].events = POLLIN|POLLHUP;
>>                  pp->pfd[i].fd = -1;
>>          }
>>          sigchain_push_common(handle_children_on_signal);
>> @@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
>>          /* Buffer output from all pipes. */
>>          for (i = 0; i < pp->max_processes; i++) {
>>                  if (pp->children[i].in_use &&
>> -                   pp->pfd[i].revents & POLLIN)
>> -                       if (strbuf_read_once(&pp->children[i].err,
>> -                                            pp->children[i].process.err, 0) < 0)
>> +                   pp->pfd[i].revents & (POLLIN|POLLHUP)) {
>> +                       int n = strbuf_read_once(&pp->children[i].err,
>> +                                            pp->children[i].process.err, 0);
>> +                       if (n == 0) {
>> +                               close(pp->children[i].process.err);
>> +                               pp->children[i].process.err = -1;
>
> So you set .err to -1 to signal the process has ended here...
>
>> -
>>                  for (i = 0; i < pp->max_processes; i++)
>>                          if (pp->children[i].in_use &&
>> -                           pid == pp->children[i].process.pid)
>> +                           pp->children[i].process.err == -1)
>>                                  break;
>
> to make a decision here if we want to finish_command on it.

Correct.

>> +               code = finish_command(&pp->children[i].process);
>
>> -               child_process_clear(&pp->children[i].process);
>
> but .err stays stays -1 here for the next iteration?
> We would need to reset it to 0 again.

No. In the next round, we need -1 to request a pipe. get_next_task 
callback sets it to -1 as well (and I think it is wrong that the 
callback does it; pp_start_one should do that).

> So .err is
>    0 when the slot is not in use
>   -1 when the child has finished awaiting termination
>   >0 when the child is living a happy life.

But, as I said, .err is not the right place to mark dying processes (it 
was just the simplest way to demonstrate the concept in this patch). 
Better extend .in_use to a tri-state indicator.

-- Hannes