run_command: report system call errors instead of returning error codes
The motivation for this change is that system call failures are serious errors that should be reported to the user, but only few callers took the burden to decode the error codes that the functions returned into error messages. If at all, then only an unspecific error message was given. A prominent example is this: $ git upload-pack . | : fatal: unable to run 'git-upload-pack' In this example, git-upload-pack, the external command invoked through the git wrapper, dies due to SIGPIPE, but the git wrapper does not bother to report the real cause. In fact, this very error message is copied to the syslog if git-daemon's client aborts the connection early. With this change, system call failures are reported immediately after the failure and only a generic failure code is returned to the caller. In the above example the error is now to the point: $ git upload-pack . | : error: git-upload-pack died of signal Note that there is no error report if the invoked program terminated with a non-zero exit code, because it is reasonable to expect that the invoked program has already reported an error. (But many run_command call sites nevertheless write a generic error message.) There was one special return code that was used to identify the case where run_command failed because the requested program could not be exec'd. This special case is now treated like a system call failure with errno set to ENOENT. No error is reported in this case, because the call site in git.c expects this as a normal result. Therefore, the callers that carefully decoded the return value still check for this condition. Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Johannes Sixt committed
Jul 4, 2009 at 21:26 UTC
0ac77ec3150f43a5c2a6b1e47e9db5aafe53fb72
6 files changed
+58
-86
builtin-receive-pack.c
+3
-19
@@ -125,27 +125,11 @@ static const char post_receive_hook[] = "hooks/post-receive";
125
126
static int run_status(int code, const char *cmd_name)
127
{
128
- switch (code) {
129
- case 0:
130
- return 0;
131
- case -ERR_RUN_COMMAND_FORK:
132
- return error("fork of %s failed", cmd_name);
133
- case -ERR_RUN_COMMAND_EXEC:
128
+ if (code < 0 && errno == ENOENT)
129
return error("execute of %s failed", cmd_name);
135
- case -ERR_RUN_COMMAND_PIPE:
136
- return error("pipe failed");
137
- case -ERR_RUN_COMMAND_WAITPID:
138
- return error("waitpid failed");
139
- case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
140
- return error("waitpid is confused");
141
- case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
142
- return error("%s died of signal", cmd_name);
143
- case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
144
- return error("%s died strangely", cmd_name);
145
- default:
130
+ else if (code > 0)
131
error("%s exited with error code %d", cmd_name, code);
147
- return code;
148
- }
132
+ return code;
133
}
134
135
static int run_receive_hook(const char *hook_name)
git.c
+1
-5
@@ -417,12 +417,8 @@ static void execv_dashed_external(const char **argv)
417
* OK to return. Otherwise, we just pass along the status code.
418
*/
419
status = run_command_v_opt(argv, 0);
420
- if (status != -ERR_RUN_COMMAND_EXEC) {
421
- if (status < 0)
422
- die("unable to run '%s'", argv[0]);
420
+ if (status >= 0 || errno != ENOENT)
421
exit(status);
424
- }
425
- errno = ENOENT; /* as if we called execvp */
422
423
argv[0] = tmp;
424
run-command.c
+49
-40
@@ -19,6 +19,7 @@ int start_command(struct child_process *cmd)
19
{
20
int need_in, need_out, need_err;
21
int fdin[2], fdout[2], fderr[2];
22
+ int failed_errno;
23
24
/*
25
* In case of errors we must keep the promise to close FDs
@@ -28,9 +29,10 @@ int start_command(struct child_process *cmd)
29
need_in = !cmd->no_stdin && cmd->in < 0;
30
if (need_in) {
31
if (pipe(fdin) < 0) {
32
+ failed_errno = errno;
33
if (cmd->out > 0)
34
close(cmd->out);
33
- return -ERR_RUN_COMMAND_PIPE;
35
+ goto fail_pipe;
36
}
37
cmd->in = fdin[1];
38
}
@@ -40,11 +42,12 @@ int start_command(struct child_process *cmd)
42
&& cmd->out < 0;
43
if (need_out) {
44
if (pipe(fdout) < 0) {
45
+ failed_errno = errno;
46
if (need_in)
47
close_pair(fdin);
48
else if (cmd->in)
49
close(cmd->in);
47
- return -ERR_RUN_COMMAND_PIPE;
50
+ goto fail_pipe;
51
}
52
cmd->out = fdout[0];
53
}
@@ -52,6 +55,7 @@ int start_command(struct child_process *cmd)
55
need_err = !cmd->no_stderr && cmd->err < 0;
56
if (need_err) {
57
if (pipe(fderr) < 0) {
58
+ failed_errno = errno;
59
if (need_in)
60
close_pair(fdin);
61
else if (cmd->in)
@@ -60,7 +64,11 @@ int start_command(struct child_process *cmd)
64
close_pair(fdout);
65
else if (cmd->out)
66
close(cmd->out);
63
- return -ERR_RUN_COMMAND_PIPE;
67
+fail_pipe:
68
+ error("cannot create pipe for %s: %s",
69
+ cmd->argv[0], strerror(failed_errno));
70
+ errno = failed_errno;
71
+ return -1;
72
}
73
cmd->err = fderr[0];
74
}
@@ -122,6 +130,9 @@ int start_command(struct child_process *cmd)
130
strerror(errno));
131
exit(127);
132
}
133
+ if (cmd->pid < 0)
134
+ error("cannot fork() for %s: %s", cmd->argv[0],
135
+ strerror(failed_errno = errno));
136
#else
137
int s0 = -1, s1 = -1, s2 = -1; /* backups of stdin, stdout, stderr */
138
const char **sargv = cmd->argv;
@@ -173,6 +184,9 @@ int start_command(struct child_process *cmd)
184
}
185
186
cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
187
+ failed_errno = errno;
188
+ if (cmd->pid < 0 && errno != ENOENT)
189
+ error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
190
191
if (cmd->env)
192
free_environ(env);
@@ -189,7 +203,6 @@ int start_command(struct child_process *cmd)
203
#endif
204
205
if (cmd->pid < 0) {
192
- int err = errno;
206
if (need_in)
207
close_pair(fdin);
208
else if (cmd->in)
@@ -200,9 +213,8 @@ int start_command(struct child_process *cmd)
213
close(cmd->out);
214
if (need_err)
215
close_pair(fderr);
203
- return err == ENOENT ?
204
- -ERR_RUN_COMMAND_EXEC :
205
- -ERR_RUN_COMMAND_FORK;
216
+ errno = failed_errno;
217
+ return -1;
218
}
219
220
if (need_in)
@@ -221,33 +233,41 @@ int start_command(struct child_process *cmd)
233
return 0;
234
}
235
224
-static int wait_or_whine(pid_t pid)
236
+static int wait_or_whine(pid_t pid, const char *argv0)
237
{
226
- for (;;) {
227
- int status, code;
228
- pid_t waiting = waitpid(pid, &status, 0);
229
-
230
- if (waiting < 0) {
231
- if (errno == EINTR)
232
- continue;
233
- error("waitpid failed (%s)", strerror(errno));
234
- return -ERR_RUN_COMMAND_WAITPID;
235
- }
236
- if (waiting != pid)
237
- return -ERR_RUN_COMMAND_WAITPID_WRONG_PID;
238
- if (WIFSIGNALED(status))
239
- return -ERR_RUN_COMMAND_WAITPID_SIGNAL;
240
-
241
- if (!WIFEXITED(status))
242
- return -ERR_RUN_COMMAND_WAITPID_NOEXIT;
238
+ int status, code = -1;
239
+ pid_t waiting;
240
+ int failed_errno = 0;
241
+
242
+ while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
243
+ ; /* nothing */
244
+
245
+ if (waiting < 0) {
246
+ failed_errno = errno;
247
+ error("waitpid for %s failed: %s", argv0, strerror(errno));
248
+ } else if (waiting != pid) {
249
+ error("waitpid is confused (%s)", argv0);
250
+ } else if (WIFSIGNALED(status)) {
251
+ error("%s died of signal", argv0);
252
+ } else if (WIFEXITED(status)) {
253
code = WEXITSTATUS(status);
244
- return code == 127 ? -ERR_RUN_COMMAND_EXEC : code;
254
+ /*
255
+ * Convert special exit code when execvp failed.
256
+ */
257
+ if (code == 127) {
258
+ code = -1;
259
+ failed_errno = ENOENT;
260
+ }
261
+ } else {
262
+ error("waitpid is confused (%s)", argv0);
263
}
264
+ errno = failed_errno;
265
+ return code;
266
}
267
268
int finish_command(struct child_process *cmd)
269
{
250
- return wait_or_whine(cmd->pid);
270
+ return wait_or_whine(cmd->pid, cmd->argv[0]);
271
}
272
273
int run_command(struct child_process *cmd)
@@ -331,10 +351,7 @@ int start_async(struct async *async)
351
int finish_async(struct async *async)
352
{
353
#ifndef __MINGW32__
334
- int ret = 0;
335
-
336
- if (wait_or_whine(async->pid))
337
- ret = error("waitpid (async) failed");
354
+ int ret = wait_or_whine(async->pid, "child process");
355
#else
356
DWORD ret = 0;
357
if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0)
@@ -378,15 +395,7 @@ int run_hook(const char *index_file, const char *name, ...)
395
hook.env = env;
396
}
397
381
- ret = start_command(&hook);
398
+ ret = run_command(&hook);
399
free(argv);
383
- if (ret) {
384
- warning("Could not spawn %s", argv[0]);
385
- return ret;
386
- }
387
- ret = finish_command(&hook);
388
- if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
389
- warning("%s exited due to uncaught signal", argv[0]);
390
-
400
return ret;
401
}
run-command.h
-10
@@ -1,16 +1,6 @@
1
#ifndef RUN_COMMAND_H
2
#define RUN_COMMAND_H
3
4
-enum {
5
- ERR_RUN_COMMAND_FORK = 10000,
6
- ERR_RUN_COMMAND_EXEC,
7
- ERR_RUN_COMMAND_PIPE,
8
- ERR_RUN_COMMAND_WAITPID,
9
- ERR_RUN_COMMAND_WAITPID_WRONG_PID,
10
- ERR_RUN_COMMAND_WAITPID_SIGNAL,
11
- ERR_RUN_COMMAND_WAITPID_NOEXIT,
12
-};
13
-
4
struct child_process {
5
const char **argv;
6
pid_t pid;
t/t5530-upload-pack-error.sh
+4
-1
@@ -53,7 +53,10 @@ test_expect_success 'upload-pack fails due to error in rev-list' '
53
! echo "0032want $(git rev-parse HEAD)
54
00000009done
55
0000" | git upload-pack . > /dev/null 2> output.err &&
56
- grep "waitpid (async) failed" output.err
56
+ # pack-objects survived
57
+ grep "Total.*, reused" output.err &&
58
+ # but there was an error, which must have been in rev-list
59
+ grep "bad tree object" output.err
60
'
61
62
test_expect_success 'create empty repository' '
transport.c
+1
-11
@@ -417,18 +417,8 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
417
argv[argc++] = *refspec++;
418
argv[argc] = NULL;
419
err = run_command_v_opt(argv, RUN_GIT_CMD);
420
- switch (err) {
421
- case -ERR_RUN_COMMAND_FORK:
422
- error("unable to fork for %s", argv[0]);
423
- case -ERR_RUN_COMMAND_EXEC:
420
+ if (err < 0 && errno == ENOENT)
421
error("unable to exec %s", argv[0]);
425
- break;
426
- case -ERR_RUN_COMMAND_WAITPID:
427
- case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
428
- case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
429
- case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
430
- error("%s died with strange error", argv[0]);
431
- }
422
return !!err;
423
}
424