[libvirt] PATCH: 0/5: Enhancing the virExec function
The following series of patches do a bunch of work on the virExec function. Overall this leads to a more robust implementation with better error reporting, and an increased level functionality. This lets us remove all uses of fork/exec in libvirt and replace them with virExec. This was all motivated by some fork/exec bugs discovered when refactoring LXC driver which have potential to affect every use of fork/exec in our code. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
There are several system calls in the virExec function for which we don't or can't report errors. This patch does a couple of things to improve the situation. It moves the code for setting non-block/close-exec to before the fork() so we can report errors for it. It removes the 'dom' and 'net' params from the ReportError function since we deprecated those long ago and all callers simply pass in NULL. It resets the 'virErrorHandler' callback to NULL in the child, so that errors raised will get reported to stderr instead of invoking a callback which is likely no longer valid in the child process. It reports failures to exec the binary and dup file descriptors. All errors in the child will end up on stderr, so they will at least be visible on the parent's console, or a logfile if one was setup for the child. Previously there would just be silent failure. Daniel diff -r 4f44b07c47c1 src/util.c --- a/src/util.c Mon Aug 11 20:45:28 2008 +0100 +++ b/src/util.c Tue Aug 12 14:52:41 2008 +0100 @@ -61,9 +61,7 @@ #ifndef PROXY static void ReportError(virConnectPtr conn, - virDomainPtr dom, - virNetworkPtr net, - int code, const char *fmt, ...) { + int code, const char *fmt, ...) { va_list args; char errorMessage[MAX_ERROR_LEN]; @@ -74,7 +72,7 @@ } else { errorMessage[0] = '\0'; } - __virRaiseError(conn, dom, net, VIR_FROM_NONE, code, VIR_ERR_ERROR, + __virRaiseError(conn, NULL, NULL, VIR_FROM_NONE, code, VIR_ERR_ERROR, NULL, NULL, NULL, -1, -1, "%s", errorMessage); } @@ -109,7 +107,7 @@ int pipeerr[2] = {-1,-1}; if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) { - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot open %s: %s"), _PATH_DEVNULL, strerror(errno)); goto cleanup; @@ -117,13 +115,41 @@ if ((outfd != NULL && pipe(pipeout) < 0) || (errfd != NULL && pipe(pipeerr) < 0)) { - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot create pipe: %s"), strerror(errno)); goto cleanup; } + if (outfd) { + if(non_block && + virSetNonBlock(pipeout[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + + if(virSetCloseExec(pipeout[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + } + if (errfd) { + if(non_block && + virSetNonBlock(pipeerr[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + if(virSetCloseExec(pipeerr[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + } + if ((pid = fork()) < 0) { - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot fork child process: %s"), strerror(errno)); goto cleanup; } @@ -132,26 +158,10 @@ close(null); if (outfd) { close(pipeout[1]); - if(non_block) - if(virSetNonBlock(pipeout[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - - if(virSetCloseExec(pipeout[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); *outfd = pipeout[0]; } if (errfd) { close(pipeerr[1]); - if(non_block) - if(virSetNonBlock(pipeerr[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - - if(virSetCloseExec(pipeerr[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); *errfd = pipeerr[0]; } *retpid = pid; @@ -160,23 +170,47 @@ /* child */ - if (pipeout[0] > 0 && close(pipeout[0]) < 0) + /* Don't want to report errors against this accidentally, so + just discard it */ + conn = NULL; + /* Remove any error callback too, so errors in child now + get sent to stderr where they stand a fighting chance + of being seen / logged */ + virSetErrorFunc(NULL, NULL); + + if (pipeout[0] > 0) + close(pipeout[0]); + if (pipeerr[0] > 0) + close(pipeerr[0]); + + + if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stdin file handle: %s"), strerror(errno)); _exit(1); - if (pipeerr[0] > 0 && close(pipeerr[0]) < 0) + } +#ifndef ENABLE_DEBUG + if (dup2(pipeout[1] > 0 ? pipeout[1] : null, STDOUT_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stdout file handle: %s"), strerror(errno)); _exit(1); - - if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) + } + if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stderr file handle: %s"), strerror(errno)); _exit(1); -#ifndef ENABLE_DEBUG - if (dup2(pipeout[1] > 0 ? pipeout[1] : null, STDOUT_FILENO) < 0) + } +#else /* ENABLE_DEBUG */ + if (pipeout[1] > 0 && dup2(pipeout[1], STDOUT_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stderr file handle: %s"), strerror(errno)); _exit(1); - if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0) + } + if (pipeerr[1] > 0 && dup2(pipeerr[1], STDERR_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stdout file handle: %s"), strerror(errno)); _exit(1); -#else /* ENABLE_DEBUG */ - if (pipeout[1] > 0 && dup2(pipeout[1], STDOUT_FILENO) < 0) - _exit(1); - if (pipeerr[1] > 0 && dup2(pipeerr[1], STDERR_FILENO) < 0) - _exit(1); + } #endif /* ENABLE_DEBUG */ close(null); @@ -187,11 +221,21 @@ execvp(argv[0], (char **) argv); + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot execute binary '%s': %s"), + argv[0], strerror(errno)); + _exit(1); return 0; cleanup: + /* This is cleanup of parent process only - child + should never jump here on error */ + + /* NB we don't ReportError() on any failures here + because the code which jumped hre already raised + an error condition which we must not overwrite */ if (pipeerr[0] > 0) close(pipeerr[0]); if (pipeerr[1] > 0) @@ -246,12 +290,22 @@ return ret; while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR); - if (ret == -1) + if (ret == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot wait for '%s': %s"), + argv[0], strerror(errno)); return -1; + } if (status == NULL) { errno = EINVAL; - return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1; + if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) + return 0; + + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("%s exited with non-zero status %s"), + argv[0]); + return -1; } else { *status = exitstatus; return 0; @@ -268,7 +322,7 @@ int *outfd ATTRIBUTE_UNUSED, int *errfd ATTRIBUTE_UNUSED) { - ReportError (conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); + ReportError (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); return -1; } @@ -280,7 +334,7 @@ int *outfd ATTRIBUTE_UNUSED, int *errfd ATTRIBUTE_UNUSED) { - ReportError (conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); + ReportError (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); return -1; } -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
There are several system calls in the virExec function for which we don't or can't report errors. This patch does a couple of things to improve the situation. It moves the code for setting non-block/close-exec to before the fork() so we can report errors for it. It removes the 'dom' and 'net' params from the ReportError function since we deprecated those long ago and all callers simply pass in NULL. It resets the 'virErrorHandler' callback to NULL in the child, so that errors raised will get reported to stderr instead of invoking a callback which is likely no longer valid in the child process. It reports failures to exec the binary and dup file descriptors. All errors in the child will end up on stderr, so they will at least be visible on the parent's console, or a logfile if one was setup for the child. Previously there would just be silent failure.
Daniel
Related question: is there any practical way to return error output from a virRun command in a libvirt error message? In testing some of the storage code I hit a few bugs where we improperly called out to another app, and the raised libvirt error had no output. I would have to manually run libvirtd and watch what output the commands dumped. I guess the other option would be to set up log files for the different storage operations similar to how qemu domain logfiles work, or maybe just a general libvirtd output log. - Cole
On Wed, Aug 13, 2008 at 10:21:59AM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
There are several system calls in the virExec function for which we don't or can't report errors. This patch does a couple of things to improve the situation. It moves the code for setting non-block/close-exec to before the fork() so we can report errors for it. It removes the 'dom' and 'net' params from the ReportError function since we deprecated those long ago and all callers simply pass in NULL. It resets the 'virErrorHandler' callback to NULL in the child, so that errors raised will get reported to stderr instead of invoking a callback which is likely no longer valid in the child process. It reports failures to exec the binary and dup file descriptors. All errors in the child will end up on stderr, so they will at least be visible on the parent's console, or a logfile if one was setup for the child. Previously there would just be silent failure.
Daniel
Related question: is there any practical way to return error output from a virRun command in a libvirt error message?
In testing some of the storage code I hit a few bugs where we improperly called out to another app, and the raised libvirt error had no output. I would have to manually run libvirtd and watch what output the commands dumped.
Yes, I think we'd want to have virRun call virExec() setting up a pair of pipes for stderr/out. virRun would then read data from those pipes and stick into a string. The string could be returned to the caller, or just included in a libvirt error raised upon failure with non-zero exit status.
I guess the other option would be to set up log files for the different storage operations similar to how qemu domain logfiles work, or maybe just a general libvirtd output log.
I think we need to either feed it back to the caller directly, or raise an error. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
There are several system calls in the virExec function for which we don't or can't report errors. This patch does a couple of things to improve the situation. It moves the code for setting non-block/close-exec to before the fork() so we can report errors for it. It removes the 'dom' and 'net' params from the ReportError function since we deprecated those long ago and all callers simply pass in NULL. It resets the 'virErrorHandler' callback to NULL in the child, so that errors raised will get reported to stderr instead of invoking a callback which is likely no longer valid in the child process. It reports failures to exec the binary and dup file descriptors. All errors in the child will end up on stderr, so they will at least be visible on the parent's console, or a logfile if one was setup for the child. Previously there would just be silent failure.
Looks fine. ACK one "would be nice" comment:
diff -r 4f44b07c47c1 src/util.c ... if (status == NULL) { errno = EINVAL; - return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1; + if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) + return 0; + + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("%s exited with non-zero status %s"),
Since this code is going to be used in so many contexts, now, it'd be nice to report which signal (if any) and the precise exit status value.
+ argv[0]); + return -1;
On Tue, Aug 19, 2008 at 12:44:16PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
There are several system calls in the virExec function for which we don't or can't report errors. This patch does a couple of things to improve the situation. It moves the code for setting non-block/close-exec to before the fork() so we can report errors for it. It removes the 'dom' and 'net' params from the ReportError function since we deprecated those long ago and all callers simply pass in NULL. It resets the 'virErrorHandler' callback to NULL in the child, so that errors raised will get reported to stderr instead of invoking a callback which is likely no longer valid in the child process. It reports failures to exec the binary and dup file descriptors. All errors in the child will end up on stderr, so they will at least be visible on the parent's console, or a logfile if one was setup for the child. Previously there would just be silent failure.
Looks fine. ACK
one "would be nice" comment:
diff -r 4f44b07c47c1 src/util.c ... if (status == NULL) { errno = EINVAL; - return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1; + if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) + return 0; + + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("%s exited with non-zero status %s"),
Since this code is going to be used in so many contexts, now, it'd be nice to report which signal (if any) and the precise exit status value.
Yes, good idea - i'll add that when i commit. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This is the same patch from yesterday to fix the signal handler race condition. We block signals before doing a fork(), and then in the child reset all signal handlers before finally unblocking all signals. The parent will restore its original signal mask after fork. I've fixed the incorrect return code check on pthread_sigmask() and iterate from 1 instead of 0. I'll switch to pid_t later, since that'll involve changing all callers too. In the internal.h file I also #define pthread_sigmask to sigprocmask for scenarios where we don't have pthread - as per other usage. This exposed a bug in remote_protocol.c file where it was not including the config.h file, hence that change here too qemud/remote_protocol.c | 1 qemud/remote_protocol.h | 1 qemud/remote_protocol.x | 1 src/internal.h | 1 src/util.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 58 insertions(+), 1 deletion(-) Daniel diff -r 76da44084eee qemud/remote_protocol.c --- a/qemud/remote_protocol.c Tue Aug 12 22:10:07 2008 +0100 +++ b/qemud/remote_protocol.c Wed Aug 13 10:07:22 2008 +0100 @@ -4,6 +4,7 @@ */ #include "remote_protocol.h" +#include <config.h> #include "internal.h" bool_t diff -r 76da44084eee qemud/remote_protocol.h --- a/qemud/remote_protocol.h Tue Aug 12 22:10:07 2008 +0100 +++ b/qemud/remote_protocol.h Wed Aug 13 10:07:22 2008 +0100 @@ -13,6 +13,7 @@ extern "C" { #endif +#include <config.h> #include "internal.h" #define REMOTE_MESSAGE_MAX 262144 #define REMOTE_STRING_MAX 65536 diff -r 76da44084eee qemud/remote_protocol.x --- a/qemud/remote_protocol.x Tue Aug 12 22:10:07 2008 +0100 +++ b/qemud/remote_protocol.x Wed Aug 13 10:07:22 2008 +0100 @@ -36,6 +36,7 @@ * 'REMOTE_'. This makes names quite long. */ +%#include <config.h> %#include "internal.h" /*----- Data types. -----*/ diff -r 76da44084eee src/internal.h --- a/src/internal.h Tue Aug 12 22:10:07 2008 +0100 +++ b/src/internal.h Wed Aug 13 10:07:22 2008 +0100 @@ -22,6 +22,7 @@ #define pthread_mutex_destroy(lk) /*empty*/ #define pthread_mutex_lock(lk) /*empty*/ #define pthread_mutex_unlock(lk) /*empty*/ +#define pthread_sigmask(h, s, o) sigprocmask((h), (s), (o)) #endif /* The library itself is allowed to use deprecated functions / diff -r 76da44084eee src/util.c --- a/src/util.c Tue Aug 12 22:10:07 2008 +0100 +++ b/src/util.c Wed Aug 13 10:07:22 2008 +0100 @@ -37,6 +37,7 @@ #include <sys/wait.h> #endif #include <string.h> +#include <signal.h> #include "c-ctype.h" #ifdef HAVE_PATHS_H @@ -49,6 +50,10 @@ #include "util.h" #include "memory.h" #include "util-lib.c" + +#ifndef NSIG +# define NSIG 32 +#endif #ifndef MIN # define MIN(a, b) ((a) < (b) ? (a) : (b)) @@ -102,9 +107,23 @@ _virExec(virConnectPtr conn, const char *const*argv, int *retpid, int infd, int *outfd, int *errfd, int non_block) { - int pid, null; + int pid, null, i; int pipeout[2] = {-1,-1}; int pipeerr[2] = {-1,-1}; + sigset_t oldmask, newmask; + struct sigaction sig_action; + + /* + * Need to block signals now, so that child process can safely + * kill off caller's signal handlers without a race. + */ + sigfillset(&newmask); + if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot block signals: %s"), + strerror(errno)); + return -1; + } if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) { ReportError(conn, VIR_ERR_INTERNAL_ERROR, @@ -164,6 +183,16 @@ close(pipeerr[1]); *errfd = pipeerr[0]; } + + /* Restore our original signal mask now child is safely + running */ + if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot unblock signals: %s"), + strerror(errno)); + return -1; + } + *retpid = pid; return 0; } @@ -177,6 +206,30 @@ get sent to stderr where they stand a fighting chance of being seen / logged */ virSetErrorFunc(NULL, NULL); + + /* Clear out all signal handlers from parent so nothing + unexpected can happen in our child once we unblock + signals */ + sig_action.sa_handler = SIG_DFL; + sig_action.sa_flags = 0; + sigemptyset(&sig_action.sa_mask); + + for (i = 1 ; i < NSIG ; i++) + /* Only possible errors are EFAULT or EINVAL + The former wont happen, the latter we + expect, so no need to check return value */ + sigaction(i, &sig_action, NULL); + + /* Unmask all signals in child, since we've no idea + what the caller's done with their signal mask + and don't want to propagate that to children */ + sigemptyset(&newmask); + if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot unblock signals: %s"), + strerror(errno)); + return -1; + } if (pipeout[0] > 0) close(pipeout[0]); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This is the same patch from yesterday to fix the signal handler race condition. We block signals before doing a fork(), and then in the child reset all signal handlers before finally unblocking all signals. The parent will restore its original signal mask after fork. I've fixed the incorrect return code check on pthread_sigmask() and iterate from 1 instead of 0. I'll switch to pid_t later, since that'll involve changing all callers too.
In the internal.h file I also #define pthread_sigmask to sigprocmask for scenarios where we don't have pthread - as per other usage. This exposed a bug in remote_protocol.c file where it was not including the config.h file, hence that change here too
This looks fine now. ACK.
qemud/remote_protocol.c | 1 qemud/remote_protocol.h | 1 qemud/remote_protocol.x | 1 src/internal.h | 1
On Tue, Aug 19, 2008 at 06:25:01PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This is the same patch from yesterday to fix the signal handler race condition. We block signals before doing a fork(), and then in the child reset all signal handlers before finally unblocking all signals. The parent will restore its original signal mask after fork. I've fixed the incorrect return code check on pthread_sigmask() and iterate from 1 instead of 0. I'll switch to pid_t later, since that'll involve changing all callers too.
In the internal.h file I also #define pthread_sigmask to sigprocmask for scenarios where we don't have pthread - as per other usage. This exposed a bug in remote_protocol.c file where it was not including the config.h file, hence that change here too
This looks fine now.
Thanks, this is committed Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
The contract for virExec() currently allows the caller to pass in a NULL for stdout/err parameters in which case the child will be connected to /dev/null, or they can pass in a pointer to an int, in which case the child will be connected to a pair of pipes, and the read end of the pipe is returned to the caller. The LXC driver will require a 3rd option - we want to pass in a existing file handler connected to a logfile. So this patch extends the semantics of these two parameters. If the stderr/out params are non-NULL, but are initialized to -1, then a pipe will be allocated. If they are >= 0, then they are assumed to be existing FDs to dup onto the child's stdout/err. This change neccessitated updating all existing callers of virExec to make sure they initialize the parameters to -1 to maintain existing behaviour. openvz_driver.c | 4 - qemu_driver.c | 2 util.c | 120 ++++++++++++++++++++++++++++++++------------------------ 3 files changed, 73 insertions(+), 53 deletions(-) Daniel diff -r 100b059a8488 src/openvz_driver.c --- a/src/openvz_driver.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/openvz_driver.c Tue Aug 12 22:12:42 2008 +0100 @@ -736,7 +736,7 @@ static int openvzListDomains(virConnectPtr conn, int *ids, int nids) { int got = 0; - int veid, pid, outfd, errfd; + int veid, pid, outfd = -1, errfd = -1; int ret; char buf[32]; char *endptr; @@ -772,7 +772,7 @@ static int openvzListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { int got = 0; - int veid, pid, outfd, errfd, ret; + int veid, pid, outfd = -1, errfd = -1, ret; char vpsname[OPENVZ_NAME_MAX]; char buf[32]; char *endptr; diff -r 100b059a8488 src/qemu_driver.c --- a/src/qemu_driver.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/qemu_driver.c Tue Aug 12 22:12:42 2008 +0100 @@ -948,6 +948,8 @@ if (safewrite(vm->logfile, "\n", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), errno, strerror(errno)); + + vm->stdout_fd = vm->stderr_fd = -1; ret = virExecNonBlock(conn, argv, &vm->pid, vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd); diff -r 100b059a8488 src/util.c --- a/src/util.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/util.c Tue Aug 12 22:12:42 2008 +0100 @@ -110,6 +110,7 @@ int pid, null, i; int pipeout[2] = {-1,-1}; int pipeerr[2] = {-1,-1}; + int childout = -1, childerr = -1; sigset_t oldmask, newmask; struct sigaction sig_action; @@ -132,39 +133,66 @@ goto cleanup; } - if ((outfd != NULL && pipe(pipeout) < 0) || - (errfd != NULL && pipe(pipeerr) < 0)) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("cannot create pipe: %s"), strerror(errno)); - goto cleanup; + if (outfd != NULL) { + if (*outfd == -1) { + if (pipe(pipeout) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot create pipe: %s"), strerror(errno)); + goto cleanup; + } + + if (non_block && + virSetNonBlock(pipeout[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + + if (virSetCloseExec(pipeout[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + + childout = pipeout[1]; + } else { + childout = *outfd; + } +#ifndef ENABLE_DEBUG + } else { + childout = null; +#endif } - if (outfd) { - if(non_block && - virSetNonBlock(pipeout[0]) == -1) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - goto cleanup; + if (errfd != NULL) { + if (*errfd == -1) { + if (pipe(pipeerr) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot create pipe: %s"), strerror(errno)); + goto cleanup; + } + + if (non_block && + virSetNonBlock(pipeerr[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + + if (virSetCloseExec(pipeerr[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + + childerr = pipeerr[1]; + } else { + childerr = *errfd; } - - if(virSetCloseExec(pipeout[0]) == -1) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); - goto cleanup; - } - } - if (errfd) { - if(non_block && - virSetNonBlock(pipeerr[0]) == -1) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - goto cleanup; - } - if(virSetCloseExec(pipeerr[0]) == -1) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); - goto cleanup; - } +#ifndef ENABLE_DEBUG + } else { + childerr = null; +#endif } if ((pid = fork()) < 0) { @@ -175,11 +203,11 @@ if (pid) { /* parent */ close(null); - if (outfd) { + if (outfd && *outfd == -1) { close(pipeout[1]); *outfd = pipeout[0]; } - if (errfd) { + if (errfd && *errfd == -1) { close(pipeerr[1]); *errfd = pipeerr[0]; } @@ -242,35 +270,25 @@ _("failed to setup stdin file handle: %s"), strerror(errno)); _exit(1); } -#ifndef ENABLE_DEBUG - if (dup2(pipeout[1] > 0 ? pipeout[1] : null, STDOUT_FILENO) < 0) { + if (childout > 0 && + dup2(childout, STDOUT_FILENO) < 0) { ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("failed to setup stdout file handle: %s"), strerror(errno)); _exit(1); } - if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0) { + if (childerr > 0 && + dup2(childerr, STDERR_FILENO) < 0) { ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("failed to setup stderr file handle: %s"), strerror(errno)); _exit(1); } -#else /* ENABLE_DEBUG */ - if (pipeout[1] > 0 && dup2(pipeout[1], STDOUT_FILENO) < 0) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("failed to setup stderr file handle: %s"), strerror(errno)); - _exit(1); - } - if (pipeerr[1] > 0 && dup2(pipeerr[1], STDERR_FILENO) < 0) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("failed to setup stdout file handle: %s"), strerror(errno)); - _exit(1); - } -#endif /* ENABLE_DEBUG */ close(null); - if (pipeout[1] > 0) - close(pipeout[1]); - if (pipeerr[1] > 0) - close(pipeerr[1]); + if (childout > 0) + close(childout); + if (childerr > 0 && + childerr != childout) + close(childerr); execvp(argv[0], (char **) argv); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
The contract for virExec() currently allows the caller to pass in a NULL for stdout/err parameters in which case the child will be connected to /dev/null, or they can pass in a pointer to an int, in which case the child will be connected to a pair of pipes, and the read end of the pipe is returned to the caller.
The LXC driver will require a 3rd option - we want to pass in a existing file handler connected to a logfile. So this patch extends the semantics of these two parameters. If the stderr/out params are non-NULL, but are initialized to -1, then a pipe will be allocated. If they are >= 0, then they are assumed to be existing FDs to dup onto the child's stdout/err.
This change neccessitated updating all existing callers of virExec to make sure they initialize the parameters to -1 to maintain existing behaviour.
ACK No technical problems... so here are some stylistic suggestions
diff -r 100b059a8488 src/openvz_driver.c --- a/src/openvz_driver.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/openvz_driver.c Tue Aug 12 22:12:42 2008 +0100 @@ -736,7 +736,7 @@
static int openvzListDomains(virConnectPtr conn, int *ids, int nids) { int got = 0; - int veid, pid, outfd, errfd; + int veid, pid, outfd = -1, errfd = -1;
I find this formatting a little easier to read/maintain: [e.g., with each declaration on a separate line, independent changes to veid and errfd might not conflict, but they surely would when they're all on the same line. ] int veid; int pid; int outfd = -1; int errfd = -1;
int ret; char buf[32]; char *endptr; @@ -772,7 +772,7 @@ static int openvzListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { int got = 0; - int veid, pid, outfd, errfd, ret; + int veid, pid, outfd = -1, errfd = -1, ret; char vpsname[OPENVZ_NAME_MAX]; char buf[32]; char *endptr; diff -r 100b059a8488 src/qemu_driver.c --- a/src/qemu_driver.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/qemu_driver.c Tue Aug 12 22:12:42 2008 +0100 @@ -948,6 +948,8 @@ if (safewrite(vm->logfile, "\n", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), errno, strerror(errno)); + + vm->stdout_fd = vm->stderr_fd = -1;
Same here, putting the common bits one above the other makes it easier to see the parts that vary: vm->stdout_fd = -1; vm->stderr_fd = -1;
ret = virExecNonBlock(conn, argv, &vm->pid, vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd); diff -r 100b059a8488 src/util.c --- a/src/util.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/util.c Tue Aug 12 22:12:42 2008 +0100 @@ -110,6 +110,7 @@ int pid, null, i; int pipeout[2] = {-1,-1}; int pipeerr[2] = {-1,-1}; + int childout = -1, childerr = -1;
And here: int childout = -1; int childerr = -1;
sigset_t oldmask, newmask; struct sigaction sig_action;
@@ -132,39 +133,66 @@ goto cleanup; }
- if ((outfd != NULL && pipe(pipeout) < 0) || - (errfd != NULL && pipe(pipeerr) < 0)) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("cannot create pipe: %s"), strerror(errno)); - goto cleanup; + if (outfd != NULL) { + if (*outfd == -1) { + if (pipe(pipeout) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot create pipe: %s"), strerror(errno));
Maybe s/cannot/Failed to/? The latter is slightly stronger/clearer, and consistent with other messages.
+ goto cleanup; + }
On Tue, Aug 19, 2008 at 10:00:37PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
The contract for virExec() currently allows the caller to pass in a NULL for stdout/err parameters in which case the child will be connected to /dev/null, or they can pass in a pointer to an int, in which case the child will be connected to a pair of pipes, and the read end of the pipe is returned to the caller.
The LXC driver will require a 3rd option - we want to pass in a existing file handler connected to a logfile. So this patch extends the semantics of these two parameters. If the stderr/out params are non-NULL, but are initialized to -1, then a pipe will be allocated. If they are >= 0, then they are assumed to be existing FDs to dup onto the child's stdout/err.
This change neccessitated updating all existing callers of virExec to make sure they initialize the parameters to -1 to maintain existing behaviour.
ACK No technical problems... so here are some stylistic suggestions
Committed, along with the style changes. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Some of the existing usage of fork/exec in libvirt is done such that the child process is daemonized. In particular the libvirt_proxy and the auto-spawned libvirtd for qemu:///session. Since we want to switch these to use virExec, we need to suport a daemon mode. This patch removes the two alternate virExec and virExecNonBlock functions and renames the internal __virExec to virExec. It then gains a 'flags' parameter which can be used to specify non-blocking mode, or daemon mode. We also add the ability to pass in a list of environment variables which get passed on to execve(). We also now explicitly close all file handles. Although libvirt code is careful to set O_CLOSEXEC on all its file handles, in multi-threaded apps there is a race condition between opening a FD and setting O_CLOSEXEC. Furthermore, we can't guarentee that all applications using libvirt are careful to set O_CLOSEXEC. Leaking FDs to a child is a potential security risk and often causes SELinux AVCs to be raised. Thus explicitely closing all FDs is a important safety net. openvz_driver.c | 4 +- qemu_driver.c | 7 ++-- storage_backend.c | 4 +- util.c | 78 +++++++++++++++++++++++++++++++++--------------------- util.h | 18 +++++++++--- 5 files changed, 71 insertions(+), 40 deletions(-) Daniel diff -r 28ddf9f5791c src/openvz_driver.c --- a/src/openvz_driver.c Tue Aug 12 22:12:42 2008 +0100 +++ b/src/openvz_driver.c Tue Aug 12 22:13:02 2008 +0100 @@ -742,7 +742,7 @@ char *endptr; const char *cmd[] = {VZLIST, "-ovpsid", "-H" , NULL}; - ret = virExec(conn, cmd, &pid, -1, &outfd, &errfd); + ret = virExec(conn, cmd, NULL, &pid, -1, &outfd, &errfd, VIR_EXEC_NONE); if(ret == -1) { openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VZLIST); @@ -779,7 +779,7 @@ const char *cmd[] = {VZLIST, "-ovpsid", "-H", "-S", NULL}; /* the -S options lists only stopped domains */ - ret = virExec(conn, cmd, &pid, -1, &outfd, &errfd); + ret = virExec(conn, cmd, NULL, &pid, -1, &outfd, &errfd, VIR_EXEC_NONE); if(ret == -1) { openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VZLIST); diff -r 28ddf9f5791c src/qemu_driver.c --- a/src/qemu_driver.c Tue Aug 12 22:12:42 2008 +0100 +++ b/src/qemu_driver.c Tue Aug 12 22:13:02 2008 +0100 @@ -951,8 +951,9 @@ vm->stdout_fd = vm->stderr_fd = -1; - ret = virExecNonBlock(conn, argv, &vm->pid, - vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd); + ret = virExec(conn, argv, NULL, &vm->pid, + vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd, + VIR_EXEC_NONBLOCK); if (ret == 0) { vm->def->id = driver->nextvmid++; vm->state = migrateFrom ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING; @@ -1199,7 +1200,7 @@ if (qemudBuildDnsmasqArgv(conn, network, &argv) < 0) return -1; - ret = virExecNonBlock(conn, argv, &network->dnsmasqPid, -1, NULL, NULL); + ret = virExec(conn, argv, NULL, &network->dnsmasqPid, -1, NULL, NULL, VIR_EXEC_NONBLOCK); for (i = 0; argv[i]; i++) VIR_FREE(argv[i]); diff -r 28ddf9f5791c src/storage_backend.c --- a/src/storage_backend.c Tue Aug 12 22:12:42 2008 +0100 +++ b/src/storage_backend.c Tue Aug 12 22:13:02 2008 +0100 @@ -403,7 +403,7 @@ /* Run the program and capture its output */ - if (virExec(conn, prog, &child, -1, &fd, NULL) < 0) { + if (virExec(conn, prog, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { goto cleanup; } @@ -537,7 +537,7 @@ v[i] = NULL; /* Run the program and capture its output */ - if (virExec(conn, prog, &child, -1, &fd, NULL) < 0) { + if (virExec(conn, prog, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { goto cleanup; } diff -r 28ddf9f5791c src/util.c --- a/src/util.c Tue Aug 12 22:12:42 2008 +0100 +++ b/src/util.c Tue Aug 12 22:13:02 2008 +0100 @@ -103,11 +103,14 @@ return 0; } -static int -_virExec(virConnectPtr conn, - const char *const*argv, - int *retpid, int infd, int *outfd, int *errfd, int non_block) { - int pid, null, i; +int +virExec(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + int *retpid, + int infd, int *outfd, int *errfd, + int flags) { + int pid, null, i, openmax; int pipeout[2] = {-1,-1}; int pipeerr[2] = {-1,-1}; int childout = -1, childerr = -1; @@ -141,7 +144,7 @@ goto cleanup; } - if (non_block && + if ((flags & VIR_EXEC_NONBLOCK) && virSetNonBlock(pipeout[0]) == -1) { ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("Failed to set non-blocking file descriptor flag")); @@ -172,7 +175,7 @@ goto cleanup; } - if (non_block && + if ((flags & VIR_EXEC_NONBLOCK) && virSetNonBlock(pipeerr[0]) == -1) { ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("Failed to set non-blocking file descriptor flag")); @@ -259,10 +262,40 @@ return -1; } - if (pipeout[0] > 0) - close(pipeout[0]); - if (pipeerr[0] > 0) - close(pipeerr[0]); + openmax = sysconf (_SC_OPEN_MAX); + for (i = 3; i < openmax; i++) + if (i != infd && + i != null && + i != childout && + i != childerr) + close(i); + + if (flags & VIR_EXEC_DAEMON) { + if (setsid() < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot become session leader: %s"), + strerror(errno)); + _exit(1); + } + + if (chdir("/") < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot change to root directory: %s"), + strerror(errno)); + _exit(1); + } + + pid = fork(); + if (pid < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot fork child process: %s"), + strerror(errno)); + _exit(1); + } + + if (pid > 0) + _exit(0); + } if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) { @@ -290,7 +323,10 @@ childerr != childout) close(childerr); - execvp(argv[0], (char **) argv); + if (envp) + execve(argv[0], (char **) argv, (char**)envp); + else + execvp(argv[0], (char **) argv); ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot execute binary '%s': %s"), @@ -320,22 +356,6 @@ return -1; } -int -virExec(virConnectPtr conn, - const char *const*argv, - int *retpid, int infd, int *outfd, int *errfd) { - - return(_virExec(conn, argv, retpid, infd, outfd, errfd, 0)); -} - -int -virExecNonBlock(virConnectPtr conn, - const char *const*argv, - int *retpid, int infd, int *outfd, int *errfd) { - - return(_virExec(conn, argv, retpid, infd, outfd, errfd, 1)); -} - /** * @conn connection to report errors against * @argv NULL terminated argv to run @@ -357,7 +377,7 @@ int *status) { int childpid, exitstatus, ret; - if ((ret = virExec(conn, argv, &childpid, -1, NULL, NULL)) < 0) + if ((ret = virExec(conn, argv, NULL, &childpid, -1, NULL, NULL, VIR_EXEC_NONE)) < 0) return ret; while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR); diff -r 28ddf9f5791c src/util.h --- a/src/util.h Tue Aug 12 22:12:42 2008 +0100 +++ b/src/util.h Tue Aug 12 22:13:02 2008 +0100 @@ -27,10 +27,20 @@ #include "util-lib.h" #include "verify.h" -int virExec(virConnectPtr conn, const char *const*argv, int *retpid, - int infd, int *outfd, int *errfd); -int virExecNonBlock(virConnectPtr conn, const char *const*argv, int *retpid, - int infd, int *outfd, int *errfd); +enum { + VIR_EXEC_NONE = 0, + VIR_EXEC_NONBLOCK = (1 << 0), + VIR_EXEC_DAEMON = (1 << 1), +}; + +int virExec(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + int *retpid, + int infd, + int *outfd, + int *errfd, + int flags); int virRun(virConnectPtr conn, const char *const*argv, int *status); int __virFileReadAll(const char *path, -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Some of the existing usage of fork/exec in libvirt is done such that the child process is daemonized. In particular the libvirt_proxy and the auto-spawned libvirtd for qemu:///session. Since we want to switch these to use virExec, we need to suport a daemon mode.
This patch removes the two alternate virExec and virExecNonBlock functions and renames the internal __virExec to virExec. It then gains a 'flags' parameter which can be used to specify non-blocking mode, or daemon mode.
We also add the ability to pass in a list of environment variables which get passed on to execve(). We also now explicitly close all file handles. Although libvirt code is careful to set O_CLOSEXEC on all its file handles, in multi-threaded apps there is a race condition between opening a FD and setting O_CLOSEXEC. Furthermore, we can't guarentee that all applications using libvirt are careful to set O_CLOSEXEC. Leaking FDs to a child is a potential security risk and often causes SELinux AVCs to be raised. Thus explicitely closing all FDs is a important safety net.
How about closing those FDs in the child instead, right after the fork? Then, if a virExec caller has an open socket or file descriptor, it won't be closed behind its back.
diff -r 28ddf9f5791c src/util.c ... + for (i = 3; i < openmax; i++) + if (i != infd && + i != null && + i != childout && + i != childerr) + close(i); + + if (flags & VIR_EXEC_DAEMON) { + if (setsid() < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot become session leader: %s"), + strerror(errno)); + _exit(1); + } + + if (chdir("/") < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot change to root directory: %s"), + strerror(errno)); + _exit(1); + } + + pid = fork(); ...
On Tue, Aug 19, 2008 at 10:51:29PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Some of the existing usage of fork/exec in libvirt is done such that the child process is daemonized. In particular the libvirt_proxy and the auto-spawned libvirtd for qemu:///session. Since we want to switch these to use virExec, we need to suport a daemon mode.
This patch removes the two alternate virExec and virExecNonBlock functions and renames the internal __virExec to virExec. It then gains a 'flags' parameter which can be used to specify non-blocking mode, or daemon mode.
We also add the ability to pass in a list of environment variables which get passed on to execve(). We also now explicitly close all file handles. Although libvirt code is careful to set O_CLOSEXEC on all its file handles, in multi-threaded apps there is a race condition between opening a FD and setting O_CLOSEXEC. Furthermore, we can't guarentee that all applications using libvirt are careful to set O_CLOSEXEC. Leaking FDs to a child is a potential security risk and often causes SELinux AVCs to be raised. Thus explicitely closing all FDs is a important safety net.
How about closing those FDs in the child instead, right after the fork? Then, if a virExec caller has an open socket or file descriptor, it won't be closed behind its back.
This is being done after a fork(). The diff context in this patch is a little misleading. The fork() shown here is the 2nd optional fork() done in daemon mode. The first fork() is higher up before we close the FDs.
diff -r 28ddf9f5791c src/util.c ... + for (i = 3; i < openmax; i++) + if (i != infd && + i != null && + i != childout && + i != childerr) + close(i); + + if (flags & VIR_EXEC_DAEMON) { + if (setsid() < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot become session leader: %s"), + strerror(errno)); + _exit(1); + } + + if (chdir("/") < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot change to root directory: %s"), + strerror(errno)); + _exit(1); + } + + pid = fork(); ...
Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Aug 19, 2008 at 10:51:29PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Some of the existing usage of fork/exec in libvirt is done such that the child process is daemonized. In particular the libvirt_proxy and the auto-spawned libvirtd for qemu:///session. Since we want to switch these to use virExec, we need to suport a daemon mode.
This patch removes the two alternate virExec and virExecNonBlock functions and renames the internal __virExec to virExec. It then gains a 'flags' parameter which can be used to specify non-blocking mode, or daemon mode.
We also add the ability to pass in a list of environment variables which get passed on to execve(). We also now explicitly close all file handles. Although libvirt code is careful to set O_CLOSEXEC on all its file handles, in multi-threaded apps there is a race condition between opening a FD and setting O_CLOSEXEC. Furthermore, we can't guarentee that all applications using libvirt are careful to set O_CLOSEXEC. Leaking FDs to a child is a potential security risk and often causes SELinux AVCs to be raised. Thus explicitely closing all FDs is a important safety net.
How about closing those FDs in the child instead, right after the fork? Then, if a virExec caller has an open socket or file descriptor, it won't be closed behind its back.
This is being done after a fork(). The diff context in this patch is a little misleading. The fork() shown here is the 2nd optional fork() done in daemon mode. The first fork() is higher up before we close the FDs.
I should have known. Next time I'll review with the full context. ACK, then.
On Tue, Aug 19, 2008 at 11:10:49PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Aug 19, 2008 at 10:51:29PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Some of the existing usage of fork/exec in libvirt is done such that the child process is daemonized. In particular the libvirt_proxy and the auto-spawned libvirtd for qemu:///session. Since we want to switch these to use virExec, we need to suport a daemon mode.
This patch removes the two alternate virExec and virExecNonBlock functions and renames the internal __virExec to virExec. It then gains a 'flags' parameter which can be used to specify non-blocking mode, or daemon mode.
We also add the ability to pass in a list of environment variables which get passed on to execve(). We also now explicitly close all file handles. Although libvirt code is careful to set O_CLOSEXEC on all its file handles, in multi-threaded apps there is a race condition between opening a FD and setting O_CLOSEXEC. Furthermore, we can't guarentee that all applications using libvirt are careful to set O_CLOSEXEC. Leaking FDs to a child is a potential security risk and often causes SELinux AVCs to be raised. Thus explicitely closing all FDs is a important safety net.
How about closing those FDs in the child instead, right after the fork? Then, if a virExec caller has an open socket or file descriptor, it won't be closed behind its back.
This is being done after a fork(). The diff context in this patch is a little misleading. The fork() shown here is the 2nd optional fork() done in daemon mode. The first fork() is higher up before we close the FDs.
I should have known. Next time I'll review with the full context. ACK, then.
Ok, committed now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This final patch switches over all places which do fork()/exec() to use the new enhanced virExec() code. A fair amount of code is deleted, but that's not the whole story - the new impl is more robust that most of the existing code we're deleting here. bridge.c | 51 +++------------- proxy_internal.c | 25 +------- qemu_conf.c | 168 ++++++++++++++++++++++-------------------------------- remote_internal.c | 88 ++++------------------------ 4 files changed, 100 insertions(+), 232 deletions(-) Daniel diff -r 2591ebc40bd7 src/bridge.c --- a/src/bridge.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/bridge.c Tue Aug 12 15:33:42 2008 +0100 @@ -46,6 +46,7 @@ #include "internal.h" #include "memory.h" +#include "util.h" #define MAX_BRIDGE_ID 256 @@ -596,42 +597,6 @@ return brGetInetAddr(ctl, ifname, SIOCGIFNETMASK, addr, maxlen); } -static int -brctlSpawn(char * const *argv) -{ - pid_t pid, ret; - int status; - int null = -1; - - if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) - return errno; - - pid = fork(); - if (pid == -1) { - int saved_errno = errno; - close(null); - return saved_errno; - } - - if (pid == 0) { /* child */ - dup2(null, STDIN_FILENO); - dup2(null, STDOUT_FILENO); - dup2(null, STDERR_FILENO); - close(null); - - execvp(argv[0], argv); - - _exit (1); - } - - close(null); - - while ((ret = waitpid(pid, &status, 0) == -1) && errno == EINTR); - if (ret == -1) - return errno; - - return (WIFEXITED(status) && WEXITSTATUS(status) == 0) ? 0 : EINVAL; -} /** * brSetForwardDelay: @@ -649,7 +614,7 @@ const char *bridge, int delay) { - char **argv; + const char **argv; int retval = ENOMEM; int n; char delayStr[30]; @@ -680,7 +645,10 @@ argv[n++] = NULL; - retval = brctlSpawn(argv); + if (virRun(NULL, argv, NULL) < 0) + retval = errno; + else + retval = 0; error: if (argv) { @@ -709,7 +677,7 @@ const char *bridge, int enable) { - char **argv; + const char **argv; int retval = ENOMEM; int n; @@ -737,7 +705,10 @@ argv[n++] = NULL; - retval = brctlSpawn(argv); + if (virRun(NULL, argv, NULL) < 0) + retval = errno; + else + retval = 0; error: if (argv) { diff -r 2591ebc40bd7 src/proxy_internal.c --- a/src/proxy_internal.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/proxy_internal.c Tue Aug 12 15:33:42 2008 +0100 @@ -162,6 +162,7 @@ { const char *proxyPath = virProxyFindServerPath(); int ret, pid, status; + const char *proxyarg[2]; if (!proxyPath) { fprintf(stderr, "failed to find libvirt_proxy\n"); @@ -171,27 +172,11 @@ if (debug) fprintf(stderr, "Asking to launch %s\n", proxyPath); - /* Become a daemon */ - pid = fork(); - if (pid == 0) { - long open_max; - long i; + proxyarg[0] = proxyPath; + proxyarg[1] = NULL; - /* don't hold open fd opened from the client of the library */ - open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - fcntl (i, F_SETFD, FD_CLOEXEC); - - setsid(); - if (fork() == 0) { - execl(proxyPath, proxyPath, NULL); - fprintf(stderr, _("failed to exec %s\n"), proxyPath); - } - /* - * calling exit() generate troubles for termination handlers - */ - _exit(0); - } + if (virExec(NULL, proxyarg, NULL, &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + fprintf(stderr, "Failed to fork libvirt_proxy\n"); /* * do a waitpid on the intermediate process to avoid zombies. diff -r 2591ebc40bd7 src/qemu_conf.c --- a/src/qemu_conf.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/qemu_conf.c Tue Aug 12 15:33:42 2008 +0100 @@ -397,116 +397,88 @@ int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) { + const char *const qemuarg[] = { qemu, "-help", NULL }; + const char *const qemuenv[] = { "LANG=C", NULL }; pid_t child; - int newstdout[2]; + int newstdout = -1; + char help[8192]; /* Ought to be enough to hold QEMU help screen */ + int got = 0, ret = -1, status; + int major, minor, micro; + int ver; if (flags) *flags = 0; if (version) *version = 0; - if (pipe(newstdout) < 0) { + if (virExec(NULL, qemuarg, qemuenv, &child, + -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) return -1; + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = read(newstdout, help+got, sizeof(help)-got-1)) <= 0) { + if (!len) + break; + if (errno == EINTR) + continue; + goto cleanup2; + } + got += len; + } + help[got] = '\0'; + + if (sscanf(help, "QEMU PC emulator version %d.%d.%d", &major,&minor, µ) != 3) { + goto cleanup2; } - if ((child = fork()) < 0) { - close(newstdout[0]); - close(newstdout[1]); - return -1; + ver = (major * 1000 * 1000) + (minor * 1000) + micro; + if (version) + *version = ver; + if (flags) { + if (strstr(help, "-no-kqemu")) + *flags |= QEMUD_CMD_FLAG_KQEMU; + if (strstr(help, "-no-reboot")) + *flags |= QEMUD_CMD_FLAG_NO_REBOOT; + if (strstr(help, "-name")) + *flags |= QEMUD_CMD_FLAG_NAME; + if (strstr(help, "-drive")) + *flags |= QEMUD_CMD_FLAG_DRIVE; + if (strstr(help, "boot=on")) + *flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; + if (ver >= 9000) + *flags |= QEMUD_CMD_FLAG_VNC_COLON; + } + ret = 0; + + qemudDebug("Version %d %d %d Cooked version: %d, with flags ? %d", + major, minor, micro, *version, *flags); + +cleanup2: + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + qemudLog(QEMUD_ERR, + _("Unexpected exit status from qemu %d pid %lu"), + status, (unsigned long)child); + ret = -1; + } + /* Check & log unexpected exit status, but don't fail, + * as there's really no need to throw an error if we did + * actually read a valid version number above */ + if (WEXITSTATUS(status) != 0) { + qemudLog(QEMUD_WARN, + _("Unexpected exit status '%d', qemu probably failed"), + status); } - if (child == 0) { /* Kid */ - /* Just in case QEMU is translated someday we force to C locale.. */ - const char *const qemuenv[] = { "LANG=C", NULL }; - - if (close(STDIN_FILENO) < 0) - goto cleanup1; - if (close(STDERR_FILENO) < 0) - goto cleanup1; - if (close(newstdout[0]) < 0) - goto cleanup1; - if (dup2(newstdout[1], STDOUT_FILENO) < 0) - goto cleanup1; - - /* Passing -help, rather than relying on no-args which doesn't - always work */ - execle(qemu, qemu, "-help", (char*)NULL, qemuenv); - - cleanup1: - _exit(-1); /* Just in case */ - } else { /* Parent */ - char help[8192]; /* Ought to be enough to hold QEMU help screen */ - int got = 0, ret = -1; - int major, minor, micro; - int ver; - - if (close(newstdout[1]) < 0) - goto cleanup2; - - while (got < (sizeof(help)-1)) { - int len; - if ((len = read(newstdout[0], help+got, sizeof(help)-got-1)) <= 0) { - if (!len) - break; - if (errno == EINTR) - continue; - goto cleanup2; - } - got += len; - } - help[got] = '\0'; - - if (sscanf(help, "QEMU PC emulator version %d.%d.%d", &major,&minor, µ) != 3) { - goto cleanup2; - } - - ver = (major * 1000 * 1000) + (minor * 1000) + micro; - if (version) - *version = ver; - if (flags) { - if (strstr(help, "-no-kqemu")) - *flags |= QEMUD_CMD_FLAG_KQEMU; - if (strstr(help, "-no-reboot")) - *flags |= QEMUD_CMD_FLAG_NO_REBOOT; - if (strstr(help, "-name")) - *flags |= QEMUD_CMD_FLAG_NAME; - if (strstr(help, "-drive")) - *flags |= QEMUD_CMD_FLAG_DRIVE; - if (strstr(help, "boot=on")) - *flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; - if (ver >= 9000) - *flags |= QEMUD_CMD_FLAG_VNC_COLON; - } - ret = 0; - - qemudDebug("Version %d %d %d Cooked version: %d, with flags ? %d", - major, minor, micro, *version, *flags); - - cleanup2: - if (close(newstdout[0]) < 0) - ret = -1; - - rewait: - if (waitpid(child, &got, 0) != child) { - if (errno == EINTR) { - goto rewait; - } - qemudLog(QEMUD_ERR, - _("Unexpected exit status from qemu %d pid %lu"), - got, (unsigned long)child); - ret = -1; - } - /* Check & log unexpected exit status, but don't fail, - * as there's really no need to throw an error if we did - * actually read a valid version number above */ - if (WEXITSTATUS(got) != 0) { - qemudLog(QEMUD_WARN, - _("Unexpected exit status '%d', qemu probably failed"), - got); - } - - return ret; - } + return ret; } int qemudExtractVersion(virConnectPtr conn, diff -r 2591ebc40bd7 src/remote_internal.c --- a/src/remote_internal.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/remote_internal.c Tue Aug 12 15:33:42 2008 +0100 @@ -72,6 +72,7 @@ #include "remote_internal.h" #include "remote_protocol.h" #include "memory.h" +#include "util.h" #define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__) #define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg) @@ -221,61 +222,21 @@ remoteForkDaemon(virConnectPtr conn) { const char *daemonPath = remoteFindDaemonPath(); + const char *daemonargs[4]; int ret, pid, status; + + daemonargs[0] = daemonPath; + daemonargs[1] = "--timeout"; + daemonargs[2] = "30"; + daemonargs[3] = NULL; if (!daemonPath) { error(conn, VIR_ERR_INTERNAL_ERROR, _("failed to find libvirtd binary")); return(-1); } - /* Become a daemon */ - pid = fork(); - if (pid == 0) { - int stdinfd = -1; - int stdoutfd = -1; - int i, open_max; - if ((stdinfd = open(_PATH_DEVNULL, O_RDONLY)) < 0) - goto cleanup; - if ((stdoutfd = open(_PATH_DEVNULL, O_WRONLY)) < 0) - goto cleanup; - if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) - goto cleanup; - if (dup2(stdoutfd, STDOUT_FILENO) != STDOUT_FILENO) - goto cleanup; - if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) - goto cleanup; - if (close(stdinfd) < 0) - goto cleanup; - stdinfd = -1; - if (close(stdoutfd) < 0) - goto cleanup; - stdoutfd = -1; - - open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - if (i != STDIN_FILENO && - i != STDOUT_FILENO && - i != STDERR_FILENO) - close(i); - - setsid(); - if (fork() == 0) { - /* Run daemon in auto-shutdown mode, so it goes away when - no longer needed by an active guest, or client */ - execl(daemonPath, daemonPath, "--timeout", "30", NULL); - } - /* - * calling exit() generate troubles for termination handlers - */ - _exit(0); - - cleanup: - if (stdoutfd != -1) - close(stdoutfd); - if (stdinfd != -1) - close(stdinfd); - _exit(-1); - } + if (virExec(NULL, daemonargs, NULL, &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + error(conn, VIR_ERR_INTERNAL_ERROR, _("failed to fork libvirtd binary")); /* * do a waitpid on the intermediate process to avoid zombies. @@ -349,7 +310,7 @@ char *name = 0, *command = 0, *sockname = 0, *netcat = 0, *username = 0; char *port = 0, *authtype = 0; int no_verify = 0, no_tty = 0; - char **cmd_argv = 0; + char **cmd_argv = NULL; /* Return code from this function, and the private data. */ int retcode = VIR_DRV_OPEN_ERROR; @@ -689,31 +650,10 @@ goto failed; } - pid = fork (); - if (pid == -1) { - error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); - goto failed; - } else if (pid == 0) { /* Child. */ - close (sv[0]); - // Connect socket (sv[1]) to stdin/stdout. - close (0); - if (dup (sv[1]) == -1) perror ("dup"); - close (1); - if (dup (sv[1]) == -1) perror ("dup"); - close (sv[1]); - - // Run the external process. - if (!cmd_argv) { - if (VIR_ALLOC_N(cmd_argv, 2) < 0) { - error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); - goto failed; - } - cmd_argv[0] = command; - cmd_argv[1] = 0; - } - execvp (command, cmd_argv); - perror (command); - _exit (1); + if (virExec(conn, (const char**)cmd_argv, NULL, + &pid, sv[1], &(sv[1]), NULL, VIR_EXEC_NONE) < 0) { + error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto failed; } /* Parent continues here. */ -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This final patch switches over all places which do fork()/exec() to use the new enhanced virExec() code. A fair amount of code is deleted, but that's not the whole story - the new impl is more robust that most of the existing code we're deleting here.
Nice clean-up!
diff -r 2591ebc40bd7 src/bridge.c ... @@ -680,7 +645,10 @@
argv[n++] = NULL;
- retval = brctlSpawn(argv); + if (virRun(NULL, argv, NULL) < 0) + retval = errno;
Using errno here isn't always kosher, since _virExec doesn't always preserve it when things go wrong (the ReportError call and close calls after "cleanup:" can clobber errno). But one can argue that it doesn't matter too much, since virExec does diagnose each failure. However, that would suggest not using errno upon virRun failure. ...
- retval = brctlSpawn(argv); + if (virRun(NULL, argv, NULL) < 0) + retval = errno;
Likewise. ...
diff -r 2591ebc40bd7 src/qemu_conf.c --- a/src/qemu_conf.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/qemu_conf.c Tue Aug 12 15:33:42 2008 +0100 @@ -397,116 +397,88 @@
int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) { + const char *const qemuarg[] = { qemu, "-help", NULL }; + const char *const qemuenv[] = { "LANG=C", NULL };
If you have to use just one environment variable, use LC_ALL=C. It trumps all others, when it comes to locale-related things.
pid_t child; - int newstdout[2]; + int newstdout = -1; + char help[8192]; /* Ought to be enough to hold QEMU help screen */ + int got = 0, ret = -1, status; + int major, minor, micro; + int ver;
If you make the above 4 variables unsigned and adjust the %d formats accordingly, the version parsing will be a little tighter.
if (flags) *flags = 0; if (version) *version = 0;
- if (pipe(newstdout) < 0) { + if (virExec(NULL, qemuarg, qemuenv, &child, + -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) return -1; + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = read(newstdout, help+got, sizeof(help)-got-1)) <= 0) { + if (!len) + break; + if (errno == EINTR) + continue; + goto cleanup2; + } + got += len; + }
Any reason not to use saferead in place of this while-loop?
+ help[got] = '\0'; + + if (sscanf(help, "QEMU PC emulator version %d.%d.%d", &major,&minor, µ) != 3) { + goto cleanup2; }
- if ((child = fork()) < 0) { - close(newstdout[0]); - close(newstdout[1]); - return -1; + ver = (major * 1000 * 1000) + (minor * 1000) + micro; + if (version) + *version = ver; + if (flags) { + if (strstr(help, "-no-kqemu")) + *flags |= QEMUD_CMD_FLAG_KQEMU; + if (strstr(help, "-no-reboot")) + *flags |= QEMUD_CMD_FLAG_NO_REBOOT; + if (strstr(help, "-name")) + *flags |= QEMUD_CMD_FLAG_NAME; + if (strstr(help, "-drive")) + *flags |= QEMUD_CMD_FLAG_DRIVE; + if (strstr(help, "boot=on")) + *flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; + if (ver >= 9000) + *flags |= QEMUD_CMD_FLAG_VNC_COLON; + } + ret = 0; + + qemudDebug("Version %d %d %d Cooked version: %d, with flags ? %d",
If version can really be NULL, as the above "if (version)" tests suggest, then you'll want to change "*version" here, to e.g., version ? *version : "NA" Same for *flags.
+ major, minor, micro, *version, *flags); + +cleanup2: + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + qemudLog(QEMUD_ERR, + _("Unexpected exit status from qemu %d pid %lu"), + status, (unsigned long)child);
You've probably already fixed this, but "status" here is a combination of exit status and signal number.
+ ret = -1; + } + /* Check & log unexpected exit status, but don't fail, + * as there's really no need to throw an error if we did + * actually read a valid version number above */ + if (WEXITSTATUS(status) != 0) { + qemudLog(QEMUD_WARN, + _("Unexpected exit status '%d', qemu probably failed"), + status); } ... int qemudExtractVersion(virConnectPtr conn, diff -r 2591ebc40bd7 src/remote_internal.c --- a/src/remote_internal.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/remote_internal.c Tue Aug 12 15:33:42 2008 +0100 @@ -72,6 +72,7 @@ #include "remote_internal.h" #include "remote_protocol.h" #include "memory.h" +#include "util.h"
#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__) #define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg) @@ -221,61 +222,21 @@ remoteForkDaemon(virConnectPtr conn) { const char *daemonPath = remoteFindDaemonPath(); + const char *daemonargs[4]; int ret, pid, status; + + daemonargs[0] = daemonPath; + daemonargs[1] = "--timeout"; + daemonargs[2] = "30";
Save an arg slot and use "--timeout=30"? That's slightly more readable, too.
+ daemonargs[3] = NULL;
...
+ if (virExec(NULL, daemonargs, NULL, &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + error(conn, VIR_ERR_INTERNAL_ERROR, _("failed to fork libvirtd binary"));
s/ binary//, in case someday it's not a binary. ...
+ if (virExec(conn, (const char**)cmd_argv, NULL, + &pid, sv[1], &(sv[1]), NULL, VIR_EXEC_NONE) < 0) { + error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno));
Similar to the issue with virRun, whenever virExec returns nonzero, it has already diagnosed the error and errno may be clobbered.
On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This final patch switches over all places which do fork()/exec() to use the new enhanced virExec() code. A fair amount of code is deleted, but that's not the whole story - the new impl is more robust that most of the existing code we're deleting here.
Nice clean-up!
I believe I've addressed all the comments you had - its quite a significant change from the previous patch. I was in fact able to simplify the bridge.c file even more than I did before. bridge.c | 128 ++++-------------------------------- proxy_internal.c | 26 +------ qemu_conf.c | 188 +++++++++++++++++++++++------------------------------- qemu_conf.h | 8 +- qemu_driver.c | 15 +--- remote_internal.c | 101 +++-------------------------- 6 files changed, 125 insertions(+), 341 deletions(-) Daniel diff -r c3d345142e1b src/bridge.c --- a/src/bridge.c Wed Aug 27 12:45:11 2008 +0100 +++ b/src/bridge.c Wed Aug 27 14:09:26 2008 +0100 @@ -46,6 +46,7 @@ #include "internal.h" #include "memory.h" +#include "util.h" #define MAX_BRIDGE_ID 256 @@ -596,42 +597,6 @@ return brGetInetAddr(ctl, ifname, SIOCGIFNETMASK, addr, maxlen); } -static int -brctlSpawn(char * const *argv) -{ - pid_t pid, ret; - int status; - int null = -1; - - if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) - return errno; - - pid = fork(); - if (pid == -1) { - int saved_errno = errno; - close(null); - return saved_errno; - } - - if (pid == 0) { /* child */ - dup2(null, STDIN_FILENO); - dup2(null, STDOUT_FILENO); - dup2(null, STDERR_FILENO); - close(null); - - execvp(argv[0], argv); - - _exit (1); - } - - close(null); - - while ((ret = waitpid(pid, &status, 0) == -1) && errno == EINTR); - if (ret == -1) - return errno; - - return (WIFEXITED(status) && WEXITSTATUS(status) == 0) ? 0 : EINVAL; -} /** * brSetForwardDelay: @@ -641,7 +606,7 @@ * * Set the bridge forward delay * - * Returns 0 in case of success or an errno code in case of failure. + * Returns 0 in case of success or -1 on failure */ int @@ -649,48 +614,17 @@ const char *bridge, int delay) { - char **argv; - int retval = ENOMEM; - int n; char delayStr[30]; - - n = 1 + /* brctl */ - 1 + /* setfd */ - 1 + /* brige name */ - 1; /* value */ + const char *const progargv[] = { + BRCTL, "setfd", bridge, delayStr, NULL + }; snprintf(delayStr, sizeof(delayStr), "%d", delay); - if (VIR_ALLOC_N(argv, n + 1) < 0) - goto error; + if (virRun(NULL, progargv, NULL) < 0) + return -1; - n = 0; - - if (!(argv[n++] = strdup(BRCTL))) - goto error; - - if (!(argv[n++] = strdup("setfd"))) - goto error; - - if (!(argv[n++] = strdup(bridge))) - goto error; - - if (!(argv[n++] = strdup(delayStr))) - goto error; - - argv[n++] = NULL; - - retval = brctlSpawn(argv); - - error: - if (argv) { - n = 0; - while (argv[n]) - VIR_FREE(argv[n++]); - VIR_FREE(argv); - } - - return retval; + return 0; } /** @@ -702,52 +636,22 @@ * Control whether the bridge participates in the spanning tree protocol, * in general don't disable it without good reasons. * - * Returns 0 in case of success or an errno code in case of failure. + * Returns 0 in case of success or -1 on failure */ int brSetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED, const char *bridge, int enable) { - char **argv; - int retval = ENOMEM; - int n; + const char *setting = enable ? "on" : "off"; + const char *const progargv[] = { + BRCTL, "stp", bridge, setting, NULL + }; - n = 1 + /* brctl */ - 1 + /* stp */ - 1 + /* brige name */ - 1; /* value */ + if (virRun(NULL, progargv, NULL) < 0) + return -1; - if (VIR_ALLOC_N(argv, n + 1) < 0) - goto error; - - n = 0; - - if (!(argv[n++] = strdup(BRCTL))) - goto error; - - if (!(argv[n++] = strdup("stp"))) - goto error; - - if (!(argv[n++] = strdup(bridge))) - goto error; - - if (!(argv[n++] = strdup(enable ? "on" : "off"))) - goto error; - - argv[n++] = NULL; - - retval = brctlSpawn(argv); - - error: - if (argv) { - n = 0; - while (argv[n]) - VIR_FREE(argv[n++]); - VIR_FREE(argv); - } - - return retval; + return 0; } #endif /* WITH_QEMU || WITH_LXC */ diff -r c3d345142e1b src/proxy_internal.c --- a/src/proxy_internal.c Wed Aug 27 12:45:11 2008 +0100 +++ b/src/proxy_internal.c Wed Aug 27 14:09:26 2008 +0100 @@ -160,6 +160,7 @@ { const char *proxyPath = virProxyFindServerPath(); int ret, pid, status; + const char *proxyarg[2]; if (!proxyPath) { fprintf(stderr, "failed to find libvirt_proxy\n"); @@ -169,27 +170,12 @@ if (debug) fprintf(stderr, "Asking to launch %s\n", proxyPath); - /* Become a daemon */ - pid = fork(); - if (pid == 0) { - long open_max; - long i; + proxyarg[0] = proxyPath; + proxyarg[1] = NULL; - /* don't hold open fd opened from the client of the library */ - open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - fcntl (i, F_SETFD, FD_CLOEXEC); - - setsid(); - if (fork() == 0) { - execl(proxyPath, proxyPath, NULL); - fprintf(stderr, _("failed to exec %s\n"), proxyPath); - } - /* - * calling exit() generate troubles for termination handlers - */ - _exit(0); - } + if (virExec(NULL, proxyarg, NULL, NULL, + &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + fprintf(stderr, "Failed to fork libvirt_proxy\n"); /* * do a waitpid on the intermediate process to avoid zombies. diff -r c3d345142e1b src/qemu_conf.c --- a/src/qemu_conf.c Wed Aug 27 12:45:11 2008 +0100 +++ b/src/qemu_conf.c Wed Aug 27 14:09:26 2008 +0100 @@ -394,124 +394,100 @@ } -int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) { +int qemudExtractVersionInfo(const char *qemu, + unsigned int *retversion, + unsigned int *retflags) { + const char *const qemuarg[] = { qemu, "-help", NULL }; + const char *const qemuenv[] = { "LC_ALL=C", NULL }; pid_t child; - int newstdout[2]; + int newstdout = -1; + char help[8192]; /* Ought to be enough to hold QEMU help screen */ + int got = 0, ret = -1, status; + unsigned int major, minor, micro; + unsigned int version; + unsigned int flags = 0; - if (flags) - *flags = 0; - if (version) - *version = 0; + if (retflags) + *retflags = 0; + if (retversion) + *retversion = 0; - if (pipe(newstdout) < 0) { + if (virExec(NULL, qemuarg, qemuenv, NULL, + &child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) return -1; + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0) + goto cleanup2; + if (!len) + break; + got += len; + } + help[got] = '\0'; + + if (sscanf(help, "QEMU PC emulator version %u.%u.%u", + &major, &minor, µ) != 3) { + goto cleanup2; } - if ((child = fork()) < 0) { - close(newstdout[0]); - close(newstdout[1]); - return -1; + version = (major * 1000 * 1000) + (minor * 1000) + micro; + + if (strstr(help, "-no-kqemu")) + flags |= QEMUD_CMD_FLAG_KQEMU; + if (strstr(help, "-no-reboot")) + flags |= QEMUD_CMD_FLAG_NO_REBOOT; + if (strstr(help, "-name")) + flags |= QEMUD_CMD_FLAG_NAME; + if (strstr(help, "-drive")) + flags |= QEMUD_CMD_FLAG_DRIVE; + if (strstr(help, "boot=on")) + flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; + if (version >= 9000) + flags |= QEMUD_CMD_FLAG_VNC_COLON; + + + if (retversion) + *retversion = version; + if (retflags) + *retflags = flags; + + ret = 0; + + qemudDebug("Version %d %d %d Cooked version: %d, with flags ? %d", + major, minor, micro, version, flags); + +cleanup2: + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + qemudLog(QEMUD_ERR, + _("Unexpected exit status from qemu %d pid %lu"), + WEXITSTATUS(status), (unsigned long)child); + ret = -1; + } + /* Check & log unexpected exit status, but don't fail, + * as there's really no need to throw an error if we did + * actually read a valid version number above */ + if (WEXITSTATUS(status) != 0) { + qemudLog(QEMUD_WARN, + _("Unexpected exit status '%d', qemu probably failed"), + WEXITSTATUS(status)); } - if (child == 0) { /* Kid */ - /* Just in case QEMU is translated someday we force to C locale.. */ - const char *const qemuenv[] = { "LANG=C", NULL }; - - if (close(STDIN_FILENO) < 0) - goto cleanup1; - if (close(STDERR_FILENO) < 0) - goto cleanup1; - if (close(newstdout[0]) < 0) - goto cleanup1; - if (dup2(newstdout[1], STDOUT_FILENO) < 0) - goto cleanup1; - - /* Passing -help, rather than relying on no-args which doesn't - always work */ - execle(qemu, qemu, "-help", (char*)NULL, qemuenv); - - cleanup1: - _exit(-1); /* Just in case */ - } else { /* Parent */ - char help[8192]; /* Ought to be enough to hold QEMU help screen */ - int got = 0, ret = -1; - int major, minor, micro; - int ver; - - if (close(newstdout[1]) < 0) - goto cleanup2; - - while (got < (sizeof(help)-1)) { - int len; - if ((len = read(newstdout[0], help+got, sizeof(help)-got-1)) <= 0) { - if (!len) - break; - if (errno == EINTR) - continue; - goto cleanup2; - } - got += len; - } - help[got] = '\0'; - - if (sscanf(help, "QEMU PC emulator version %d.%d.%d", &major,&minor, µ) != 3) { - goto cleanup2; - } - - ver = (major * 1000 * 1000) + (minor * 1000) + micro; - if (version) - *version = ver; - if (flags) { - if (strstr(help, "-no-kqemu")) - *flags |= QEMUD_CMD_FLAG_KQEMU; - if (strstr(help, "-no-reboot")) - *flags |= QEMUD_CMD_FLAG_NO_REBOOT; - if (strstr(help, "-name")) - *flags |= QEMUD_CMD_FLAG_NAME; - if (strstr(help, "-drive")) - *flags |= QEMUD_CMD_FLAG_DRIVE; - if (strstr(help, "boot=on")) - *flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; - if (ver >= 9000) - *flags |= QEMUD_CMD_FLAG_VNC_COLON; - } - ret = 0; - - qemudDebug("Version %d %d %d Cooked version: %d, with flags ? %d", - major, minor, micro, *version, *flags); - - cleanup2: - if (close(newstdout[0]) < 0) - ret = -1; - - rewait: - if (waitpid(child, &got, 0) != child) { - if (errno == EINTR) { - goto rewait; - } - qemudLog(QEMUD_ERR, - _("Unexpected exit status from qemu %d pid %lu"), - got, (unsigned long)child); - ret = -1; - } - /* Check & log unexpected exit status, but don't fail, - * as there's really no need to throw an error if we did - * actually read a valid version number above */ - if (WEXITSTATUS(got) != 0) { - qemudLog(QEMUD_WARN, - _("Unexpected exit status '%d', qemu probably failed"), - got); - } - - return ret; - } + return ret; } int qemudExtractVersion(virConnectPtr conn, struct qemud_driver *driver) { const char *binary; struct stat sb; - int ignored; if (driver->qemuVersion > 0) return 0; @@ -529,7 +505,7 @@ return -1; } - if (qemudExtractVersionInfo(binary, &driver->qemuVersion, &ignored) < 0) { + if (qemudExtractVersionInfo(binary, &driver->qemuVersion, NULL) < 0) { return -1; } @@ -716,7 +692,7 @@ int qemudBuildCommandLine(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - int qemuCmdFlags, + unsigned int qemuCmdFlags, const char ***retargv, int **tapfds, int *ntapfds, diff -r c3d345142e1b src/qemu_conf.h --- a/src/qemu_conf.h Wed Aug 27 12:45:11 2008 +0100 +++ b/src/qemu_conf.h Wed Aug 27 14:09:26 2008 +0100 @@ -49,7 +49,7 @@ /* Main driver state */ struct qemud_driver { - int qemuVersion; + unsigned int qemuVersion; int nextvmid; virDomainObjPtr domains; @@ -86,13 +86,13 @@ int qemudExtractVersion (virConnectPtr conn, struct qemud_driver *driver); int qemudExtractVersionInfo (const char *qemu, - int *version, - int *flags); + unsigned int *version, + unsigned int *flags); int qemudBuildCommandLine (virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr dom, - int qemuCmdFlags, + unsigned int qemuCmdFlags, const char ***argv, int **tapfds, int *ntapfds, diff -r c3d345142e1b src/qemu_driver.c --- a/src/qemu_driver.c Wed Aug 27 12:45:11 2008 +0100 +++ b/src/qemu_driver.c Wed Aug 27 14:09:26 2008 +0100 @@ -312,6 +312,7 @@ qemudActive(void) { virDomainObjPtr dom = qemu_driver->domains; virNetworkObjPtr net = qemu_driver->networks; + while (dom) { if (virDomainIsActive(dom)) return 1; @@ -846,7 +847,7 @@ struct stat sb; int *tapfds = NULL; int ntapfds = 0; - int qemuCmdFlags; + unsigned int qemuCmdFlags; fd_set keepfd; FD_ZERO(&keepfd); @@ -1509,19 +1510,11 @@ } - if ((err = brSetForwardDelay(driver->brctl, network->def->bridge, network->def->delay))) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("failed to set bridge forward delay to %ld"), - network->def->delay); + if (brSetForwardDelay(driver->brctl, network->def->bridge, network->def->delay) < 0) goto err_delbr; - } - if ((err = brSetEnableSTP(driver->brctl, network->def->bridge, network->def->stp ? 1 : 0))) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("failed to set bridge STP to %s"), - network->def->stp ? "on" : "off"); + if (brSetEnableSTP(driver->brctl, network->def->bridge, network->def->stp ? 1 : 0) < 0) goto err_delbr; - } if (network->def->ipAddress && (err = brSetInetAddress(driver->brctl, network->def->bridge, network->def->ipAddress))) { diff -r c3d345142e1b src/remote_internal.c --- a/src/remote_internal.c Wed Aug 27 12:45:11 2008 +0100 +++ b/src/remote_internal.c Wed Aug 27 14:09:26 2008 +0100 @@ -72,6 +72,7 @@ #include "remote_internal.h" #include "remote_protocol.h" #include "memory.h" +#include "util.h" #define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__) #define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg) @@ -221,62 +222,17 @@ remoteForkDaemon(virConnectPtr conn) { const char *daemonPath = remoteFindDaemonPath(); + const char *const daemonargs[] = { daemonPath, "--timeout=30", NULL }; int ret, pid, status; if (!daemonPath) { error(conn, VIR_ERR_INTERNAL_ERROR, _("failed to find libvirtd binary")); - return(-1); - } - - /* Become a daemon */ - pid = fork(); - if (pid == 0) { - int stdinfd = -1; - int stdoutfd = -1; - int i, open_max; - if ((stdinfd = open(_PATH_DEVNULL, O_RDONLY)) < 0) - goto cleanup; - if ((stdoutfd = open(_PATH_DEVNULL, O_WRONLY)) < 0) - goto cleanup; - if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) - goto cleanup; - if (dup2(stdoutfd, STDOUT_FILENO) != STDOUT_FILENO) - goto cleanup; - if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) - goto cleanup; - if (close(stdinfd) < 0) - goto cleanup; - stdinfd = -1; - if (close(stdoutfd) < 0) - goto cleanup; - stdoutfd = -1; - - open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - if (i != STDIN_FILENO && - i != STDOUT_FILENO && - i != STDERR_FILENO) - close(i); - - setsid(); - if (fork() == 0) { - /* Run daemon in auto-shutdown mode, so it goes away when - no longer needed by an active guest, or client */ - execl(daemonPath, daemonPath, "--timeout", "30", NULL); - } - /* - * calling exit() generate troubles for termination handlers - */ - _exit(0); - - cleanup: - if (stdoutfd != -1) - close(stdoutfd); - if (stdinfd != -1) - close(stdinfd); - _exit(-1); - } - + return -1; + } + + if (virExec(NULL, daemonargs, NULL, NULL, + &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + return -1; /* * do a waitpid on the intermediate process to avoid zombies. */ @@ -287,7 +243,7 @@ goto retry_wait; } - return (0); + return 0; } #endif @@ -349,7 +305,7 @@ char *name = 0, *command = 0, *sockname = 0, *netcat = 0, *username = 0; char *port = 0, *authtype = 0; int no_verify = 0, no_tty = 0; - char **cmd_argv = 0; + char **cmd_argv = NULL; /* Return code from this function, and the private data. */ int retcode = VIR_DRV_OPEN_ERROR; @@ -693,40 +649,9 @@ goto failed; } - pid = fork (); - if (pid == -1) { - errorf (conn, VIR_ERR_SYSTEM_ERROR, - _("unable to fork external network transport: %s"), - strerror (errno)); - goto failed; - } else if (pid == 0) { /* Child. */ - close (sv[0]); - // Connect socket (sv[1]) to stdin/stdout. - close (0); - if (dup (sv[1]) == -1) { - perror ("dup"); - _exit(1); - } - close (1); - if (dup (sv[1]) == -1) { - perror ("dup"); - _exit(1); - } - close (sv[1]); - - // Run the external process. - if (!cmd_argv) { - if (VIR_ALLOC_N(cmd_argv, 2) < 0) { - perror("malloc"); - _exit(1); - } - cmd_argv[0] = command; - cmd_argv[1] = 0; - } - execvp (command, cmd_argv); - perror (command); - _exit (1); - } + if (virExec(conn, (const char**)cmd_argv, NULL, NULL, + &pid, sv[1], &(sv[1]), NULL, VIR_EXEC_NONE) < 0) + goto failed; /* Parent continues here. */ close (sv[1]); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This final patch switches over all places which do fork()/exec() to use the new enhanced virExec() code. A fair amount of code is deleted, but that's not the whole story - the new impl is more robust that most of the existing code we're deleting here.
Nice clean-up!
I believe I've addressed all the comments you had - its quite a significant change from the previous patch. I was in fact able to simplify the bridge.c file even more than I did before.
You did, indeed. Even nicer ;-) ...
diff -r c3d345142e1b src/qemu_conf.c --- a/src/qemu_conf.c Wed Aug 27 12:45:11 2008 +0100 +++ b/src/qemu_conf.c Wed Aug 27 14:09:26 2008 +0100 @@ -394,124 +394,100 @@ }
-int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) { +int qemudExtractVersionInfo(const char *qemu, + unsigned int *retversion, + unsigned int *retflags) { + const char *const qemuarg[] = { qemu, "-help", NULL }; + const char *const qemuenv[] = { "LC_ALL=C", NULL }; pid_t child; - int newstdout[2]; + int newstdout = -1; + char help[8192]; /* Ought to be enough to hold QEMU help screen */ + int got = 0, ret = -1, status; + unsigned int major, minor, micro; + unsigned int version; + unsigned int flags = 0;
- if (flags) - *flags = 0; - if (version) - *version = 0; + if (retflags) + *retflags = 0; + if (retversion) + *retversion = 0;
- if (pipe(newstdout) < 0) { + if (virExec(NULL, qemuarg, qemuenv, NULL, + &child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) return -1; + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0) + goto cleanup2; + if (!len) + break; + got += len; + } + help[got] = '\0';
I was going to ask why you are using saferead, then realized that *I* suggested the s/read+EINTR/saferead/ change. Now, while re-reviewing this, I wondered if we could get rid of the 8KB stack buffer and encapsulate the above loop -- at the expense of allocating the memory instead -- by using e.g., virFileReadAll. But virFileReadAll operates on a file name, not a file descriptor. So I wrote/factored a couple of wrappers, and now, with the following patch, you can use this: char *help = NULL; enum { MAX_HELP_OUTPUT_SIZE = 8192 }; int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help); if (len < 0) goto ... ... Then add this somewhere after done with "help": VIR_FREE(help); Go ahead and commit your changes. After you do that, I'll rebase this and propose a complete patch.
From 620584c1effef8882687c0560dcfaec5a4614c25 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 28 Aug 2008 15:54:03 +0200 Subject: [PATCH] util.c: add a file-descriptor-based wrapper for fread_file_lim
* src/util.c (virFileReadLimFP): New function. (__virFileReadLimFD): New function. * src/util.h (__virFileReadLimFD): Declare. (virFileReadLimFD): Define. (virFileReadAll): Rewrite to use virFileReadLimFP. --- src/util.c | 71 +++++++++++++++++++++++++++++++++++++++-------------------- src/util.h | 7 +++-- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/util.c b/src/util.c index a81af07..fd30778 100644 --- a/src/util.c +++ b/src/util.c @@ -510,40 +510,63 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) return NULL; } -int __virFileReadAll(const char *path, int maxlen, char **buf) +/* A wrapper around fread_file_lim that maps a failure due to + exceeding the maximum size limitation to EOVERFLOW. */ +static int virFileReadLimFP(FILE *fp, int maxlen, char **buf) { - FILE *fh; - int ret = -1; size_t len; - char *s; + char *s = fread_file_lim (fp, maxlen+1, &len); + if (s == NULL) + return -1; + if (len > maxlen || (int)len != len) { + VIR_FREE(s); + /* There was at least one byte more than MAXLEN. + Set errno accordingly. */ + errno = EOVERFLOW; + return -1; + } + *buf = s; + return len; +} + +/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */ +int __virFileReadLimFD(int fd_arg, int maxlen, char **buf) +{ + int fd = dup (fd_arg); + if (0 <= fd) { + FILE *fp = fdopen (fd, "r"); + if (fp) { + int len = virFileReadLimFP (fp, maxlen, buf); + int saved_errno = errno; + fclose (fp); + errno = saved_errno; + return len; + } else { + int saved_errno = errno; + close (fd); + errno = saved_errno; + } + } + return -1; +} - if (!(fh = fopen(path, "r"))) { +int __virFileReadAll(const char *path, int maxlen, char **buf) +{ + FILE *fh = fopen(path, "r"); + if (fh == NULL) { virLog("Failed to open file '%s': %s\n", path, strerror(errno)); - goto error; + return -1; } - s = fread_file_lim(fh, maxlen+1, &len); - if (s == NULL) { + int len = virFileReadLimFP (fh, maxlen, buf); + fclose(fh); + if (len < 0) { virLog("Failed to read '%s': %s\n", path, strerror (errno)); - goto error; - } - - if (len > maxlen || (int)len != len) { - VIR_FREE(s); - virLog("File '%s' is too large %d, max %d\n", - path, (int)len, maxlen); - goto error; + return -1; } - *buf = s; - ret = len; - - error: - if (fh) - fclose(fh); - - return ret; + return len; } int virFileMatchesNameSuffix(const char *file, diff --git a/src/util.h b/src/util.h index 5151582..f2da006 100644 --- a/src/util.h +++ b/src/util.h @@ -45,9 +45,10 @@ int virExec(virConnectPtr conn, int flags); int virRun(virConnectPtr conn, const char *const*argv, int *status); -int __virFileReadAll(const char *path, - int maxlen, - char **buf); +int __virFileReadLimFD(int fd, int maxlen, char **buf); +#define virFileReadLimFD(fd,m,b) __virFileReadLimFD((fd),(m),(b)) + +int __virFileReadAll(const char *path, int maxlen, char **buf); #define virFileReadAll(p,m,b) __virFileReadAll((p),(m),(b)) int virFileMatchesNameSuffix(const char *file, -- 1.6.0.1.126.ga118
On Thu, Aug 28, 2008 at 04:18:08PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote: + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0) + goto cleanup2; + if (!len) + break; + got += len; + } + help[got] = '\0';
I was going to ask why you are using saferead, then realized that *I* suggested the s/read+EINTR/saferead/ change. Now, while re-reviewing this, I wondered if we could get rid of the 8KB stack buffer and encapsulate the above loop -- at the expense of allocating the memory instead -- by using e.g., virFileReadAll. But virFileReadAll operates on a file name, not a file descriptor. So I wrote/factored a couple of wrappers, and now, with the following patch, you can use this:
char *help = NULL; enum { MAX_HELP_OUTPUT_SIZE = 8192 }; int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help); if (len < 0) goto ... ...
Then add this somewhere after done with "help": VIR_FREE(help);
This sounds like a nice idea - the loop is rather unpleasant to read as it is. I'll commit my patch shortly
diff --git a/src/util.c b/src/util.c index a81af07..fd30778 100644 --- a/src/util.c +++ b/src/util.c @@ -510,40 +510,63 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) return NULL; }
-int __virFileReadAll(const char *path, int maxlen, char **buf) +/* A wrapper around fread_file_lim that maps a failure due to + exceeding the maximum size limitation to EOVERFLOW. */ +static int virFileReadLimFP(FILE *fp, int maxlen, char **buf) { - FILE *fh; - int ret = -1; size_t len; - char *s; + char *s = fread_file_lim (fp, maxlen+1, &len); + if (s == NULL) + return -1; + if (len > maxlen || (int)len != len) { + VIR_FREE(s); + /* There was at least one byte more than MAXLEN. + Set errno accordingly. */ + errno = EOVERFLOW; + return -1; + } + *buf = s; + return len; +} + +/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */ +int __virFileReadLimFD(int fd_arg, int maxlen, char **buf) +{ + int fd = dup (fd_arg); + if (0 <= fd) {
Can we stick to 'fd >= 0' or 'fd < 0' or 'fd == -1'. I find the reversed constant-first conditionals rather painful to read. Always have to stop and think about them for too long. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */ +int __virFileReadLimFD(int fd_arg, int maxlen, char **buf) +{ + int fd = dup (fd_arg); + if (0 <= fd) {
Can we stick to 'fd >= 0' or 'fd < 0' or 'fd == -1'. I find the reversed constant-first conditionals rather painful to read. Always have to stop and think about them for too long.
Sure. I've just adjusted it like this: - if (0 <= fd) { + if (fd >= 0) {
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Aug 28, 2008 at 04:18:08PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote: + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0) + goto cleanup2; + if (!len) + break; + got += len; + } + help[got] = '\0';
I was going to ask why you are using saferead, then realized that *I* suggested the s/read+EINTR/saferead/ change. Now, while re-reviewing this, I wondered if we could get rid of the 8KB stack buffer and encapsulate the above loop -- at the expense of allocating the memory instead -- by using e.g., virFileReadAll. But virFileReadAll operates on a file name, not a file descriptor. So I wrote/factored a couple of wrappers, and now, with the following patch, you can use this:
char *help = NULL; enum { MAX_HELP_OUTPUT_SIZE = 8192 }; int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help); if (len < 0) goto ... ...
Then add this somewhere after done with "help": VIR_FREE(help);
This sounds like a nice idea - the loop is rather unpleasant to read as it is. I'll commit my patch shortly
Here are two change sets: The first adds two new file-reading functions in util.c and makes the existing virFileReadAll use one of them. The second implements the change suggested above.
From f5fa2a2963ecc70022b30317a29b0f769a34fc8a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 28 Aug 2008 15:54:03 +0200 Subject: [PATCH] util.c: add a file-descriptor-based wrapper for fread_file_lim
* src/util.c (virFileReadLimFP): New function. (__virFileReadLimFD): New function. * src/util.h (__virFileReadLimFD): Declare. (virFileReadLimFD): Define. (virFileReadAll): Rewrite to use virFileReadLimFP. --- src/util.c | 71 +++++++++++++++++++++++++++++++++++++++-------------------- src/util.h | 7 +++-- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/util.c b/src/util.c index cb03e7b..c724268 100644 --- a/src/util.c +++ b/src/util.c @@ -510,40 +510,63 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) return NULL; } -int __virFileReadAll(const char *path, int maxlen, char **buf) +/* A wrapper around fread_file_lim that maps a failure due to + exceeding the maximum size limitation to EOVERFLOW. */ +static int virFileReadLimFP(FILE *fp, int maxlen, char **buf) { - FILE *fh; - int ret = -1; size_t len; - char *s; + char *s = fread_file_lim (fp, maxlen+1, &len); + if (s == NULL) + return -1; + if (len > maxlen || (int)len != len) { + VIR_FREE(s); + /* There was at least one byte more than MAXLEN. + Set errno accordingly. */ + errno = EOVERFLOW; + return -1; + } + *buf = s; + return len; +} + +/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */ +int __virFileReadLimFD(int fd_arg, int maxlen, char **buf) +{ + int fd = dup (fd_arg); + if (fd >= 0) { + FILE *fp = fdopen (fd, "r"); + if (fp) { + int len = virFileReadLimFP (fp, maxlen, buf); + int saved_errno = errno; + fclose (fp); + errno = saved_errno; + return len; + } else { + int saved_errno = errno; + close (fd); + errno = saved_errno; + } + } + return -1; +} - if (!(fh = fopen(path, "r"))) { +int __virFileReadAll(const char *path, int maxlen, char **buf) +{ + FILE *fh = fopen(path, "r"); + if (fh == NULL) { virLog("Failed to open file '%s': %s\n", path, strerror(errno)); - goto error; + return -1; } - s = fread_file_lim(fh, maxlen+1, &len); - if (s == NULL) { + int len = virFileReadLimFP (fh, maxlen, buf); + fclose(fh); + if (len < 0) { virLog("Failed to read '%s': %s\n", path, strerror (errno)); - goto error; - } - - if (len > maxlen || (int)len != len) { - VIR_FREE(s); - virLog("File '%s' is too large %d, max %d\n", - path, (int)len, maxlen); - goto error; + return -1; } - *buf = s; - ret = len; - - error: - if (fh) - fclose(fh); - - return ret; + return len; } int virFileMatchesNameSuffix(const char *file, diff --git a/src/util.h b/src/util.h index 5151582..f2da006 100644 --- a/src/util.h +++ b/src/util.h @@ -45,9 +45,10 @@ int virExec(virConnectPtr conn, int flags); int virRun(virConnectPtr conn, const char *const*argv, int *status); -int __virFileReadAll(const char *path, - int maxlen, - char **buf); +int __virFileReadLimFD(int fd, int maxlen, char **buf); +#define virFileReadLimFD(fd,m,b) __virFileReadLimFD((fd),(m),(b)) + +int __virFileReadAll(const char *path, int maxlen, char **buf); #define virFileReadAll(p,m,b) __virFileReadAll((p),(m),(b)) int virFileMatchesNameSuffix(const char *file, -- 1.6.0.1.126.ga118
From e5771e49d5887e546db8fc47a7ad7785ea009eec Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 29 Aug 2008 14:43:34 +0200 Subject: [PATCH] virFileReadLimFD, virFileReadLimFP: new functions; use them
* src/qemu_conf.c (qemudExtractVersionInfo): Use virFileReadLimFD and VIR_FREE in place of an open-coded loop and a static buffer. --- src/qemu_conf.c | 21 +++++++-------------- 1 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index bab2092..0686978 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -401,8 +401,7 @@ int qemudExtractVersionInfo(const char *qemu, const char *const qemuenv[] = { "LC_ALL=C", NULL }; pid_t child; int newstdout = -1; - char help[8192]; /* Ought to be enough to hold QEMU help screen */ - int got = 0, ret = -1, status; + int ret = -1, status; unsigned int major, minor, micro; unsigned int version; unsigned int flags = 0; @@ -416,16 +415,11 @@ int qemudExtractVersionInfo(const char *qemu, &child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) return -1; - - while (got < (sizeof(help)-1)) { - int len; - if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0) - goto cleanup2; - if (!len) - break; - got += len; - } - help[got] = '\0'; + char *help = NULL; + enum { MAX_HELP_OUTPUT_SIZE = 8192 }; + int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help); + if (len < 0) + goto cleanup2; if (sscanf(help, "QEMU PC emulator version %u.%u.%u", &major, &minor, µ) != 3) { @@ -447,7 +441,6 @@ int qemudExtractVersionInfo(const char *qemu, if (version >= 9000) flags |= QEMUD_CMD_FLAG_VNC_COLON; - if (retversion) *retversion = version; if (retflags) @@ -459,6 +452,7 @@ int qemudExtractVersionInfo(const char *qemu, major, minor, micro, version, flags); cleanup2: + VIR_FREE(help); if (close(newstdout) < 0) ret = -1; @@ -1229,4 +1223,3 @@ int qemudBuildCommandLine(virConnectPtr conn, #undef ADD_ARG_LIT #undef ADD_ARG_SPACE } - -- 1.6.0.1.126.ga118
Jim Meyering <jim@meyering.net> wrote:
Here are two change sets: The first adds two new file-reading functions in util.c and makes the existing virFileReadAll use one of them.
The second implements the change suggested above.
From f5fa2a2963ecc70022b30317a29b0f769a34fc8a Mon Sep 17 00:00:00 2001
After a private ACK, I've just committed these.
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering