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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Mon, Apr 1, 2013 at 8:22 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Apr 01, 2013 at 05:58:55PM -0600, Felipe Contreras wrote:

>> Are you saying that even if we have stored the result of a waitpid
>> command, if errno is EINTR, then we should still loop waitpid()? If
>> so, I guess this would do the trick:
>
> Yes, I think that would work. Though I wonder if it is even worth
> storing EINTR at all in the first place. It tells us nothing. In fact,
> does storing any error condition really tell us anything?

Probably not, I just tried to minimize the potential behavior changes.

> The two states
> we are interested in at this point are:
>
>   1. We have reaped the child via waitpid; here is its status.
>
>   2. We have not (either we did not try, it was not dead yet, or we were
>      not able to due to an error). We should now try it again.
>
> If we got EINTR the first time around, we would likely get the "real"
> answer this time. If we get anything else (like EINVAL or ECHILD), then
> we would get the same thing again calling waitpid() later.
>
>> > We now take argv0 into wait_or_whine. But I don't see it being used.
>> > What's it for?
>>
>> It was there before:
>> -static int wait_or_whine(pid_t pid, const char *argv0)
>> +static int wait_or_whine(struct child_process *cmd, pid_t pid, const
>> char *argv0)
>
> Ah, sorry, I misread the diff. We are adding "cmd", not "argv0".

Yeah, which in fact was already there before.

> That would trigger the rest of your code in the error case, which I
> think was your original intent. But then we return "0" from
> check_command. Is that right?
>
> There are three states we can be in from calling waitpid:
>
>   1. The process is dead.
>
>   2. The process is not dead.
>
>   3. We could not determine which because waitpid returned an error.
>
> It is clear that check_command is trying to tell its caller (1) or (2);
> but what should it say in case of (3)?
>
> Naively, given how patch 2 uses it, I think it would actually make sense
> for it to return 1. That is, the semantics are "return 0 if and only if
> the pid is verified to be dead; otherwise return 1".

I thought if there was an error that constituted a failure.

> But if we know from reading waitpid(3) that waitpid should only fail due
> to EINTR, or due to bogus arguments (e.g., a pid that does not exist or
> has already been reaped), then maybe something like this makes sense:
>
>   while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
>           ; /* nothing */

But we don't want to wait synchronously here, we just want to ping.

>   /* pid definitely still going */
>   if (!waiting)
>           return 1;
>
>   /* pid definitely died */
>   if (waiting == cmd->pid) {
>           cmd->last_status.valid = 1;
>           cmd->last_status.status = status;
>           return 0;
>   }
>
>   /*
>    * this should never happen, since we handed waitpid() a single
>    * pid, so it should either return that pid, 0, or an error.
>    */
>   if (waiting > 0)
>           die("BUG: waitpid reported a random pid?");
>
>   /*
>    * otherwise, we have an error. Assume the pid is gone, since that
>    * is the only reason for waitpid to report a problem besides EINTR.
>    * We don't bother recording errno, since we can just repeat
>    * the waitpid again later.
>    */
>    return 0;

The rest makes sense.

>> >> +     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,
>>
>> No, also when waiting < 0.
>
> After the fix above, yes; in the original we would always have exited
> already.

No:

+       if (waiting != cmd->pid)
+               return 1;

If waiting < 0, waiting != cmd->pid, and therefore this return is not
triggered, and there's only one more return at the end of the
function.

Cheers.

-- 
Felipe Contreras