Re: [updated patch v2 1/2] Report exec errors from run-command
flat view
From: Johannes Sixt <hidden>
Date: 2016-06-15 22:47:57
Ilari Liusvaara schrieb:
quoted hunk ↗ jump to hunk
+static inline void force_close(int fd) +{ + int err = 0; + /* + * Retry EINTRs undefinitely, exit on EBADF immediately, other + * errors retry only up to three times (even if pipe close + * shouldn't cause other errors, but you never know with + * what broken systems may return on closed file descriptor). + * consequences of failure to close pipe here may include + * deadlocking. + */ + while (close(fd) < 0 && errno != EBADF && err < 3) + if(errno != EINTR) + err++;
What's the point to iterate on all errors except EBADF? If the close() fails once, it will fail again.
quoted hunk ↗ jump to hunk
+ /* + * Clean up the process that did the failed execution + * so no zombies remain. + */ +wait_again: + r = waitpid(cmd->pid, &ret, 0); + if (r < 0 && errno != ECHILD) + goto wait_again;
You really should iterate only on well-known errors. What's wrong with while (waitpid(pid, &status, 0) < 0 && errno == EINTR) ; /* nothing */ similar to wait_or_whine()'s call to waitpid() and to avoid goto.
quoted hunk ↗ jump to hunk
+int main(int argc, char **argv) +{ + char* procs[2]; + struct child_process proc; + memset(&proc, 0, sizeof(proc)); + + if(argc < 2) + return 1; + + if (argv[1][1] == '1') + procs[0] = "does-not-exist-62896869286"; + procs[1] = NULL; + proc.argv = (const char **)procs; + + if (!run_command(&proc)) + return 1; + if (errno != ENOENT) + return 1; + return 0; +}
This test is not specific enough: It would pass even without your change to start_command(), because finish_command() detects the ENOENT case. You really want to test that you see ENOENT after start_command() (i.e., before finish_command()). -- Hannes