On Mon, Apr 1, 2013 at 5:23 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Mon, Apr 01, 2013 at 03:46:41PM -0600, Felipe Contreras wrote:
>> -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?
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:
static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid,
int *stat_loc)
{
pid_t waiting;
if (cmd->last_wait.code) {
errno = cmd->last_wait.failed_errno;
*stat_loc = cmd->last_wait.status;
if (errno != EINTR)
return errno ? -1 : pid;
}
while ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)
; /* nothing */
return waiting;
}
>> +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?
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)
>> +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).
How about this?
if (waiting >= 0 && waiting != cmd->pid)
return 1;
>> + 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.
> Why is code set to -1? It
> seems to be used as a flag to say "this structure is valid". Should it
> be defined as "unsigned valid:1;" instead?
Yeap. I was using it for other purposes before, 'valid' would be better now.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html