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

[Jeff King <peff@peff.net>]

On Mon, Apr 01, 2013 at 03:46:41PM -0600, Felipe Contreras wrote:

> And persistent_waitpid() to recover the information from the last run.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

A little background would be nice here...what problem are we solving?

> -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, failed_errno is
always 0. We do correctly record the status. 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?

-Peff