Re: [PATCH 02/10] run-command: factor out return value computation
- From
Jeff King <peff@peff.net>
- Date
- Sep 17, 2015, 10:33 UTC
- Message-ID
- <20150917103310.GC20086@sigill.intra.peff.net>
- In-Reply-To
- <1442453948-9885-3-git-send-email-sbeller@google.com>
On Wed, Sep 16, 2015 at 06:39:00PM -0700, Stefan Beller wrote:
Show 28 quoted lines
> +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:
Show 34 quoted lines
> 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