Re: [PATCH 02/10] run-command: factor out return value computation
From: Jeff King <hidden>
Date: 2016-06-15 23:06:35
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:
quoted hunk ↗ jump to hunk
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