On 05/16/2011 11:47 AM, Laine Stump wrote: Traditionally, shells have treated execve() errors according to errno: ENOENT (or anything else that implies not found) -> 127 EACCES, EINVAL, ENOEXEC (or anything else that implies found but not executable or permissions in the way of finding) -> 126 any failure not related to an exec attempt -> 1-125 So you may want to return 127/126 for exec() failure, 125 for pthread_sigmask() failure, and so forth if you want your error messages to be consistent with the shell, env(1), nice(1), and so forth. That's a pre-existing bug. You can't read WEXITSTATUS if !WIFEXITED. ACK to the idea in this part, although you may want to use symbolic names (enum or #define) so that if you adjust the values later (such as adding 126/127 for the two types of exec failure), then you've reduced the impact to the rest of the code. For example, coreutils uses the names EXIT_ENOENT for 127 and EXIT_CANNOT_INVOKE for 126. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

This is in response to: https://bugzilla.redhat.com/show_bug.cgi?id=616060 but will also be useful in an upcoming patch which needs to retrieve the output of /etc/rc.d/init.d/netcf-transaction. Before forking, we now create a pipe, then after forking, the child dup2's both stdout and stderr to the write side of the pipe, and the parent calls fdopen() on the read side of the pipe and then calls gnulib's fread_file() before calling waitpid() to reap the process. --- src/dutil_linux.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++---- src/dutil_linux.h | 2 +- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/src/dutil_linux.c b/src/dutil_linux.c index 052221d..fbed055 100644 --- a/src/dutil_linux.c +++ b/src/dutil_linux.c @@ -47,6 +47,7 @@ #include <arpa/inet.h> #include "safe-alloc.h" +#include "read-file.h" #include "ref.h" #include "list.h" #include "netcf.h" @@ -70,15 +71,29 @@ static int exec_program(struct netcf *ncf, const char *const*argv, const char *commandline, - pid_t *pid) + pid_t *pid, + int *outfd) { sigset_t oldmask, newmask; struct sigaction sig_action; char errbuf[128]; + int pipeout[2] = {-1, -1}; /* commandline is only used for error reporting */ if (commandline == NULL) commandline = argv[0]; + + /* create a pipe to receive stdout+stderr from child */ + if (outfd) { + if (pipe(pipeout) < 0) { + report_error(ncf, NETCF_EEXEC, + "failed to create pipe while forking for '%s': %s", + commandline, strerror_r(errno, errbuf, sizeof(errbuf))); + goto error; + } + *outfd = pipeout[0]; + } + /* * Need to block signals now, so that child process can safely * kill off caller's signal handlers without a race. @@ -103,6 +118,11 @@ exec_program(struct netcf *ncf, ncf, EEXEC, "failed to restore signal mask while forking for '%s': %s", commandline, strerror_r(errno, errbuf, sizeof(errbuf))); + + /* parent doesn't use write side of the pipe */ + if (pipeout[1] >= 0) + close(pipeout[1]); + return 0; } @@ -133,6 +153,18 @@ exec_program(struct netcf *ncf, _exit(126); } + if (pipeout[1] >= 0) { + /* direct stdout and stderr to the pipe */ + if (dup2(pipeout[1], fileno(stdout)) < 0 + || dup2(pipeout[1], fileno(stderr)) < 0) { + /* return a unique code and let the parent log the error */ + _exit(125); + } + } + /* child doesn't use the read side of the pipe */ + if (pipeout[0] >= 0) + close(pipeout[0]); + /* close all open file descriptors */ int openmax = sysconf (_SC_OPEN_MAX); for (i = 3; i < openmax; i++) @@ -147,6 +179,12 @@ exec_program(struct netcf *ncf, error: /* This is cleanup of parent process only - child should never jump here on error */ + if (pipeout[0] >= 0) + close(pipeout[0]); + if (pipeout[1] >= 0) + close(pipeout[1]); + if (outfd) + *outfd = -1; return -1; } @@ -157,20 +195,42 @@ error: * return -1 * */ -int run_program(struct netcf *ncf, const char *const *argv) { +int run_program(struct netcf *ncf, const char *const *argv, char **output) +{ pid_t childpid; int exitstatus, waitret; char *argv_str; int ret = -1; char errbuf[128]; + char *outtext = NULL; + int outfd = -1; + FILE *outfile = NULL; + size_t outlen; + + if (!output) + output = &outtext; argv_str = argv_to_string(argv); ERR_NOMEM(argv_str == NULL, ncf); - exec_program(ncf, argv, argv_str, &childpid); + exec_program(ncf, argv, argv_str, &childpid, &outfd); ERR_BAIL(ncf); + outfile = fdopen(outfd, "r"); + ERR_THROW(outfile == NULL, ncf, EEXEC, + "Failed to create file stream for output while executing '%s': %s", + argv_str, strerror_r(errno, errbuf, sizeof(errbuf))); + + *output = fread_file(outfile, &outlen); + ERR_THROW(*output == NULL, ncf, EEXEC, + "Error while reading output from execution of '%s': %s", + argv_str, strerror_r(errno, errbuf, sizeof(errbuf))); + + /* finished with the stream. Close it so the child can exit. */ + fclose(outfile); + outfile = NULL; + while ((waitret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR) { /* empty loop */ @@ -190,12 +250,20 @@ int run_program(struct netcf *ncf, const char *const *argv) { ERR_THROW(WEXITSTATUS(exitstatus) == 126, ncf, EEXEC, "Running '%s' failed to reset child process signal mask", argv_str); + ERR_THROW(WEXITSTATUS(exitstatus) == 125, ncf, EEXEC, + "Running '%s' failed to dup2 child process stdout/stderr", + argv_str); ERR_THROW(WEXITSTATUS(exitstatus) != 0, ncf, EEXEC, - "Running '%s' failed with exit code %d", - argv_str, WEXITSTATUS(exitstatus)); + "Running '%s' failed with exit code %d: %s", + argv_str, WEXITSTATUS(exitstatus), *output); ret = 0; error: + if (outfile) + fclose(outfile); + else if (outfd >= 0) + close(outfd); + FREE(outtext); FREE(argv_str); return ret; } @@ -206,7 +274,7 @@ void run1(struct netcf *ncf, const char *prog, const char *arg) { prog, arg, NULL }; - run_program(ncf, argv); + run_program(ncf, argv, NULL); } /* diff --git a/src/dutil_linux.h b/src/dutil_linux.h index 0f8b14a..91c83be 100644 --- a/src/dutil_linux.h +++ b/src/dutil_linux.h @@ -40,7 +40,7 @@ struct driver { }; /* run an external program */ -int run_program(struct netcf *ncf, const char *const *argv); +int run_program(struct netcf *ncf, const char *const *argv, char **output); void run1(struct netcf *ncf, const char *prog, const char *arg); /* Add a table of transformations that the next GET_AUGEAS should run */ -- 1.7.3.4

On 05/16/2011 02:58 PM, Eric Blake wrote: Done and pushed. Thanks!