Re: [PATCH 1/4] run-command: add new check_command helper

>> -static int wait_or_whine(pid_t pid, const char *argv0)
>> +static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *stat_loc)
>> +{
>> +     if (cmd->last_wait.code) {
>> +             errno = cmd->last_wait.failed_errno;
>> +             *stat_loc = cmd->last_wait.status;
>> +             return errno ? -1 : pid;
>> +     } else {
>> +             pid_t waiting;
>> +             while ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)
>> +                     ;       /* nothing */
>> +             return waiting;
>> +     }
>> +}
>
> So it looks we are trying to save the waitpid state from a previous run
> and use the saved value. Otherwise, waitpid as normal.
>
> We loop on EINTR when we actually call waitpid(). But we don't check
> whether the saved errno is waitpid. What happens if we EINTR during the
> saved call to waitpid?

>> +static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0)
>>  {
>>       int status, code = -1;
>>       pid_t waiting;
>>       int failed_errno = 0;
>>
>> -     while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
>> -             ;       /* nothing */
>> +     waiting = persistent_waitpid(cmd, pid, &status);
>>
>>       if (waiting < 0) {
>>               failed_errno = errno;
>
> We now take argv0 into wait_or_whine. But I don't see it being used.
> What's it for?

>> +int check_command(struct child_process *cmd)
>> +{
>> +     int status;
>> +     pid_t waiting;
>> +     int failed_errno = 0;
>> +
>> +     waiting = waitpid(cmd->pid, &status, WNOHANG);
>
> This might return the pid if it has died, -1 if there was an error, or 0
> if the process still exists but hasn't died. So...
>
>> +     if (waiting != cmd->pid)
>> +             return 1;
>> +
>> +     if (waiting < 0)
>> +             failed_errno = errno;
>
> How would we ever trigger this second conditional? It makes sense to
> return 1 when "waiting == 0", as that is saying "yes, your process is
> still running" (though documenting the return either at the top of the
> function or in the commit message would be helpful)
>
> But if we get an error from waitpid, we would also return 1, which
> doesn't make sense (especially if it is something like EINTR -- I don't
> know offhand if we can get EINTR during WNOHANG. It should not block,
> but I don't know if it can race with a signal).

>> +     cmd->last_wait.code = -1;
>> +     cmd->last_wait.failed_errno = failed_errno;
>> +     cmd->last_wait.status = status;
>
> Since we can only get here when waiting == cmd->pid,