Re: [PATCH 02/10] run-command: factor out return value computation

[Jeff King <peff@peff.net>]

On Wed, Sep 16, 2015 at 06:39:00PM -0700, Stefan Beller wrote:

> +static int determine_return_value(int wait_status,
> +				  int *result,
> +				  int *error_code,
> +				  const char *argv0)
> +{
> +	if (WIFSIGNALED(wait_status)) {
> +		*result = WTERMSIG(wait_status);
> +		if (*result != SIGINT && *result != SIGQUIT)
> +			error("%s died of signal %d", argv0, *result);
> +		/*
> +		 * This return value is chosen so that code & 0xff
> +		 * mimics the exit code that a POSIX shell would report for
> +		 * a program that died from this signal.
> +		 */
> +		*result += 128;
> +	} else if (WIFEXITED(wait_status)) {
> +		*result = WEXITSTATUS(wait_status);
> +		/*
> +		 * Convert special exit code when execvp failed.
> +		 */
> +		if (*result == 127) {
> +			*result = -1;
> +			*error_code = ENOENT;
> +		}
> +	} else
> +		return 1;
> +	return 0;
> +}

Looks like we can return "0" or "1" here, and the exit code goes into
"result". But our caller:

>  static int wait_or_whine(pid_t pid, const char *argv0)
>  {
>  	int status, code = -1;
> @@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>  	if (waiting < 0) {
>  		failed_errno = errno;
>  		error("waitpid for %s failed: %s", argv0, strerror(errno));
> -	} else if (waiting != pid) {
> -		error("waitpid is confused (%s)", argv0);
> -	} else if (WIFSIGNALED(status)) {
> -		code = WTERMSIG(status);
> -		if (code != SIGINT && code != SIGQUIT)
> -			error("%s died of signal %d", argv0, code);
> -		/*
> -		 * This return value is chosen so that code & 0xff
> -		 * mimics the exit code that a POSIX shell would report for
> -		 * a program that died from this signal.
> -		 */
> -		code += 128;
> -	} else if (WIFEXITED(status)) {
> -		code = WEXITSTATUS(status);
> -		/*
> -		 * Convert special exit code when execvp failed.
> -		 */
> -		if (code == 127) {
> -			code = -1;
> -			failed_errno = ENOENT;
> -		}
>  	} else {
> -		error("waitpid is confused (%s)", argv0);
> +		if (waiting != pid
> +		   || (determine_return_value(status, &code, &failed_errno, argv0) < 0))
> +			error("waitpid is confused (%s)", argv0);
>  	}

...is looking for "< 0", which will never happen. Should the "1" above
have been "-1"?

I also wondered what happened to "code" and "failed_errno" in that case.
They are OK to access because wait_or_whine() has set them to defaults,
but I wonder if determine_return_value should do so in every branch (so
it is is clear that the values are always defined when it returns).

-Peff