[libvirt] [PATCH 0/4 v3] More virCommand conversions and cleanups

The following series converts the last user of virExec to use virCommand. The remaining functionality is then moved out of util.c and into command.c v2: Committed some patches Dropped final patch, can be submitted seperately Addressed Eric's comments v3: Commited some patches Addressed Eric's v2 comments Cole Robinson (4): qemu: Convert virExec usage to virCommand util: Remove unused virExec wrapper util: Implement virRun as a wrapper around virCommand Move virRun, virExec*, virFork to util/command cfg.mk | 3 +- src/libvirt_private.syms | 6 +- src/lxc/veth.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 1 + src/qemu/qemu_driver.c | 26 +- src/qemu/qemu_process.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/util/command.c | 565 ++++++++++++++++++++++++- src/util/command.h | 14 + src/util/ebtables.c | 2 +- src/util/pci.c | 2 +- src/util/util.c | 661 +---------------------------- src/util/util.h | 32 -- src/vmware/vmware_driver.c | 1 + 15 files changed, 608 insertions(+), 713 deletions(-) -- 1.7.4.4

v2: Have virCommand cleanup intermediate process for us v3: Preserve original FD closing behavior Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 26 ++++++++++++-------------- src/qemu/qemu_process.c | 2 +- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ccaae66..101ff04 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3277,11 +3277,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, int ret = -1; virDomainEventPtr event; int intermediatefd = -1; - pid_t intermediate_pid = -1; - int childstat; + virCommandPtr cmd = NULL; if (header->version == 2) { - const char *intermediate_argv[3] = { NULL, "-dc", NULL }; const char *prog = qemudSaveCompressionTypeToString(header->compressed); if (prog == NULL) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3291,14 +3289,17 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, } if (header->compressed != QEMUD_SAVE_FORMAT_RAW) { - intermediate_argv[0] = prog; + cmd = virCommandNewArgList(prog, "-dc", NULL); intermediatefd = *fd; *fd = -1; - if (virExec(intermediate_argv, NULL, NULL, - &intermediate_pid, intermediatefd, fd, NULL, 0) < 0) { + + virCommandSetInputFD(cmd, intermediatefd); + virCommandSetOutputFD(cmd, fd); + + if (virCommandRunAsync(cmd, NULL) < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to start decompression binary %s"), - intermediate_argv[0]); + prog); *fd = intermediatefd; goto out; } @@ -3309,7 +3310,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, ret = qemuProcessStart(conn, driver, vm, "stdio", true, *fd, path, VIR_VM_OP_RESTORE); - if (intermediate_pid != -1) { + if (intermediatefd != -1) { if (ret < 0) { /* if there was an error setting up qemu, the intermediate * process will wait forever to write to stdout, so we @@ -3317,14 +3318,10 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, */ VIR_FORCE_CLOSE(intermediatefd); VIR_FORCE_CLOSE(*fd); - kill(intermediate_pid, SIGTERM); } - /* Wait for intermediate process to exit */ - while (waitpid(intermediate_pid, &childstat, 0) == -1 && - errno == EINTR) { - /* empty */ - } + if (virCommandWait(cmd, NULL) < 0) + ret = -1; } VIR_FORCE_CLOSE(intermediatefd); @@ -3364,6 +3361,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, ret = 0; out: + virCommandFree(cmd); if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path) < 0) VIR_WARN("failed to restore save state label on %s", path); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 01b15e0..642fdfb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2321,7 +2321,7 @@ int qemuProcessStart(virConnectPtr conn, * because the child no longer exists. */ - /* The virExec process that launches the daemon failed. Pending on + /* The virCommand process that launches the daemon failed. Pending on * when it failed (we can't determine for sure), there may be * extra info in the domain log (if the hook failed for example). * -- 1.7.4.4

On 05/17/2011 09:06 AM, Cole Robinson wrote:
v2: Have virCommand cleanup intermediate process for us
v3: Preserve original FD closing behavior
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 26 ++++++++++++-------------- src/qemu/qemu_process.c | 2 +- 2 files changed, 13 insertions(+), 15 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

v3: Remove obsolete comment Signed-off-by: Cole Robinson <crobinso@redhat.com> --- cfg.mk | 1 - src/libvirt_private.syms | 1 - src/util/util.c | 35 ----------------------------------- src/util/util.h | 8 -------- 4 files changed, 0 insertions(+), 45 deletions(-) diff --git a/cfg.mk b/cfg.mk index 3a10186..d16d8a8 100644 --- a/cfg.mk +++ b/cfg.mk @@ -263,7 +263,6 @@ sc_prohibit_close: $(_sc_search_regexp) # Prefer virCommand for all child processes. -# XXX - eventually, we want to enhance this to also prohibit virExec. sc_prohibit_fork_wrappers: @prohibit='= *\<(fork|popen|system) *\(' \ halt='use virCommand for child processes' \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7b6151c..fc6eeaf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -924,7 +924,6 @@ virEnumFromString; virEnumToString; virEventAddHandle; virEventRemoveHandle; -virExec; virExecWithHook; virFileAbsPath; virFileDeletePid; diff --git a/src/util/util.c b/src/util/util.c index 89fc82b..7679834 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -752,26 +752,6 @@ virExecWithHook(const char *const*argv, return -1; } -/* - * See virExecWithHook for explanation of the arguments. - * - * Wrapper function for virExecWithHook, with a simpler set of parameters. - * Used to insulate the numerous callers from changes to virExecWithHook - * argument list. - */ -int -virExec(const char *const*argv, - const char *const*envp, - const fd_set *keepfd, - pid_t *retpid, - int infd, int *outfd, int *errfd, - int flags) -{ - return virExecWithHook(argv, envp, keepfd, retpid, - infd, outfd, errfd, - flags, NULL, NULL, NULL); -} - /** * @argv NULL terminated argv to run * @status optional variable to return exit status in @@ -881,21 +861,6 @@ virRun(const char *const *argv ATTRIBUTE_UNUSED, } int -virExec(const char *const*argv ATTRIBUTE_UNUSED, - const char *const*envp ATTRIBUTE_UNUSED, - const fd_set *keepfd ATTRIBUTE_UNUSED, - int *retpid ATTRIBUTE_UNUSED, - int infd ATTRIBUTE_UNUSED, - int *outfd ATTRIBUTE_UNUSED, - int *errfd ATTRIBUTE_UNUSED, - int flags ATTRIBUTE_UNUSED) -{ - virUtilError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virExec is not implemented for WIN32")); - return -1; -} - -int virExecWithHook(const char *const*argv ATTRIBUTE_UNUSED, const char *const*envp ATTRIBUTE_UNUSED, const fd_set *keepfd ATTRIBUTE_UNUSED, diff --git a/src/util/util.h b/src/util/util.h index e2b8eb3..3e95cae 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -69,14 +69,6 @@ int virExecWithHook(const char *const*argv, virExecHook hook, void *data, char *pidfile) ATTRIBUTE_RETURN_CHECK; -int virExec(const char *const*argv, - const char *const*envp, - const fd_set *keepfd, - pid_t *retpid, - int infd, - int *outfd, - int *errfd, - int flags) ATTRIBUTE_RETURN_CHECK; int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf); -- 1.7.4.4

On 05/17/2011 09:06 AM, Cole Robinson wrote:
v3: Remove obsolete comment
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- cfg.mk | 1 - src/libvirt_private.syms | 1 - src/util/util.c | 35 ----------------------------------- src/util/util.h | 8 -------- 4 files changed, 0 insertions(+), 45 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

v2: Simplify command building Handle command building failure v3: Remove unneeded NULL check Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/util.c | 73 +++---------------------------------------------------- 1 files changed, 4 insertions(+), 69 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 7679834..92b31e2 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -769,76 +769,11 @@ virExecWithHook(const char *const*argv, int virRun(const char *const*argv, int *status) { - pid_t childpid; - int exitstatus, execret, waitret; - int ret = -1; - int errfd = -1, outfd = -1; - char *outbuf = NULL; - char *errbuf = NULL; - char *argv_str = NULL; - - if ((argv_str = virArgvToString(argv)) == NULL) { - virReportOOMError(); - goto error; - } - VIR_DEBUG("%s", argv_str); - - if ((execret = virExecWithHook(argv, NULL, NULL, - &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { - ret = execret; - goto error; - } - - if (virPipeReadUntilEOF(outfd, errfd, &outbuf, &errbuf) < 0) { - while (waitpid(childpid, &exitstatus, 0) == -1 && errno == EINTR) - ; - goto error; - } - - if (outbuf) - VIR_DEBUG("Command stdout: %s", outbuf); - if (errbuf) - VIR_DEBUG("Command stderr: %s", errbuf); - - while ((waitret = waitpid(childpid, &exitstatus, 0) == -1) && - errno == EINTR); - if (waitret == -1) { - virReportSystemError(errno, - _("cannot wait for '%s'"), - argv[0]); - goto error; - } - - if (status == NULL) { - errno = EINVAL; - if (exitstatus) { - char *str = virCommandTranslateStatus(exitstatus); - char *argvstr = virArgvToString(argv); - if (!argv_str) { - virReportOOMError(); - goto error; - } - - virUtilError(VIR_ERR_INTERNAL_ERROR, - _("'%s' exited unexpectedly: %s"), - argv_str, NULLSTR(str)); - - VIR_FREE(str); - VIR_FREE(argvstr); - goto error; - } - } else { - *status = exitstatus; - } - - ret = 0; + int ret; + virCommandPtr cmd = virCommandNewArgs(argv); - error: - VIR_FREE(outbuf); - VIR_FREE(errbuf); - VIR_FORCE_CLOSE(outfd); - VIR_FORCE_CLOSE(errfd); + ret = virCommandRun(cmd, status); + virCommandFree(cmd); return ret; } -- 1.7.4.4

On 05/17/2011 09:06 AM, Cole Robinson wrote:
v2: Simplify command building Handle command building failure
v3: Remove unneeded NULL check
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/util.c | 73 +++---------------------------------------------------- 1 files changed, 4 insertions(+), 69 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Seems reasonable to have all command wrappers in the same place v2: Dont move SetInherit v3: Comment spelling fix Adjust WARN0 comment Remove spurious #include movement Don't include sys/types.h Combine virExec enums Signed-off-by: Cole Robinson <crobinso@redhat.com> --- cfg.mk | 2 +- src/libvirt_private.syms | 5 +- src/lxc/veth.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 1 + src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/util/command.c | 565 ++++++++++++++++++++++++++++- src/util/command.h | 14 + src/util/ebtables.c | 2 +- src/util/pci.c | 2 +- src/util/util.c | 563 +---------------------------- src/util/util.h | 24 -- src/vmware/vmware_driver.c | 1 + 13 files changed, 596 insertions(+), 589 deletions(-) diff --git a/cfg.mk b/cfg.mk index d16d8a8..5d3ef26 100644 --- a/cfg.mk +++ b/cfg.mk @@ -622,7 +622,7 @@ exclude_file_name_regexp--sc_prohibit_doubled_word = ^po/ exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ (^docs/api_extension/|^tests/qemuhelpdata/|\.(gif|ico|png)$$) -_src2=src/(util/util|libvirt|lxc/lxc_controller) +_src2=src/(util/command|libvirt|lxc/lxc_controller) exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ (^docs|^($(_src2)|tests/testutils|daemon/libvirtd)\.c$$) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc6eeaf..d996bfb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -130,6 +130,8 @@ virCommandTransferFD; virCommandTranslateStatus; virCommandWait; virCommandWriteArgLog; +virFork; +virRun; # conf.h @@ -924,7 +926,6 @@ virEnumFromString; virEnumToString; virEventAddHandle; virEventRemoveHandle; -virExecWithHook; virFileAbsPath; virFileDeletePid; virFileExists; @@ -946,7 +947,6 @@ virFileStripSuffix; virFileWaitForDevices; virFileWriteStr; virFindFileInPath; -virFork; virFormatMacAddr; virGenerateMacAddr; virGetGroupID; @@ -965,7 +965,6 @@ virParseVersionString; virPipeReadUntilEOF; virRandom; virRandomInitialize; -virRun; virSetBlocking; virSetCloseExec; virSetInherit; diff --git a/src/lxc/veth.c b/src/lxc/veth.c index a00aa23..1a96f82 100644 --- a/src/lxc/veth.c +++ b/src/lxc/veth.c @@ -21,7 +21,7 @@ #include "internal.h" #include "logging.h" #include "memory.h" -#include "util.h" +#include "command.h" #include "virterror_internal.h" #define VIR_FROM_THIS VIR_FROM_LXC diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 2ff392d..b5358ce 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -39,6 +39,7 @@ #include "nwfilter_gentech_driver.h" #include "nwfilter_ebiptables_driver.h" #include "files.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b8d4d63..88f5b87 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -41,7 +41,7 @@ #include "storage_backend_fs.h" #include "storage_conf.h" #include "storage_file.h" -#include "util.h" +#include "command.h" #include "memory.h" #include "xml.h" #include "files.h" diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 7d5adf9..4de5442 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -34,7 +34,7 @@ #include "virterror_internal.h" #include "storage_backend_logical.h" #include "storage_conf.h" -#include "util.h" +#include "command.h" #include "memory.h" #include "logging.h" #include "files.h" diff --git a/src/util/command.c b/src/util/command.c index ebb90cb..e69d2a7 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -27,6 +27,11 @@ #include <stdlib.h> #include <sys/stat.h> #include <sys/wait.h> +#include <fcntl.h> + +#if HAVE_CAPNG +# include <cap-ng.h> +#endif #include "command.h" #include "memory.h" @@ -35,6 +40,7 @@ #include "logging.h" #include "files.h" #include "buf.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -42,9 +48,13 @@ virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) +/* Flags for virExecWithHook */ enum { - /* Internal-use extension beyond public VIR_EXEC_ flags */ - VIR_EXEC_RUN_SYNC = 0x40000000, + VIR_EXEC_NONE = 0, + VIR_EXEC_NONBLOCK = (1 << 0), + VIR_EXEC_DAEMON = (1 << 1), + VIR_EXEC_CLEAR_CAPS = (1 << 2), + VIR_EXEC_RUN_SYNC = (1 << 3), }; struct _virCommand { @@ -85,6 +95,557 @@ struct _virCommand { bool reap; }; +#ifndef WIN32 + +# if HAVE_CAPNG +static int virClearCapabilities(void) +{ + int ret; + + capng_clear(CAPNG_SELECT_BOTH); + + if ((ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) { + virCommandError(VIR_ERR_INTERNAL_ERROR, + _("cannot clear process capabilities %d"), ret); + return -1; + } + + return 0; +} +# else +static int virClearCapabilities(void) +{ +// VIR_WARN("libcap-ng support not compiled in, unable to clear " +// "capabilities"); + return 0; +} +# endif + + +/* virFork() - fork a new process while avoiding various race/deadlock + conditions + + @pid - a pointer to a pid_t that will receive the return value from + fork() + + on return from virFork(), if *pid < 0, the fork failed and there is + no new process. Otherwise, just like fork(), if *pid == 0, it is the + child process returning, and if *pid > 0, it is the parent. + + Even if *pid >= 0, if the return value from virFork() is < 0, it + indicates a failure that occurred in the parent or child process + after the fork. In this case, the child process should call + _exit(EXIT_FAILURE) after doing any additional error reporting. + + */ +int virFork(pid_t *pid) { +# ifdef HAVE_PTHREAD_SIGMASK + sigset_t oldmask, newmask; +# endif + struct sigaction sig_action; + int saved_errno, ret = -1; + + *pid = -1; + + /* + * Need to block signals now, so that child process can safely + * kill off caller's signal handlers without a race. + */ +# ifdef HAVE_PTHREAD_SIGMASK + sigfillset(&newmask); + if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) { + saved_errno = errno; + virReportSystemError(errno, + "%s", _("cannot block signals")); + goto cleanup; + } +# endif + + /* Ensure we hold the logging lock, to protect child processes + * from deadlocking on another thread's inherited mutex state */ + virLogLock(); + + *pid = fork(); + saved_errno = errno; /* save for caller */ + + /* Unlock for both parent and child process */ + virLogUnlock(); + + if (*pid < 0) { +# ifdef HAVE_PTHREAD_SIGMASK + /* attempt to restore signal mask, but ignore failure, to + avoid obscuring the fork failure */ + ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); +# endif + virReportSystemError(saved_errno, + "%s", _("cannot fork child process")); + goto cleanup; + } + + if (*pid) { + + /* parent process */ + +# ifdef HAVE_PTHREAD_SIGMASK + /* Restore our original signal mask now that the child is + safely running */ + if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) { + saved_errno = errno; /* save for caller */ + virReportSystemError(errno, "%s", _("cannot unblock signals")); + goto cleanup; + } +# endif + ret = 0; + + } else { + + /* child process */ + + int logprio; + int i; + + /* Remove any error callback so errors in child now + get sent to stderr where they stand a fighting chance + of being seen / logged */ + virSetErrorFunc(NULL, NULL); + virSetErrorLogPriorityFunc(NULL); + + /* Make sure any hook logging is sent to stderr, since child + * process may close the logfile FDs */ + logprio = virLogGetDefaultPriority(); + virLogReset(); + virLogSetDefaultPriority(logprio); + + /* 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); + } + +# ifdef HAVE_PTHREAD_SIGMASK + /* 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) { + saved_errno = errno; /* save for caller */ + virReportSystemError(errno, "%s", _("cannot unblock signals")); + goto cleanup; + } +# endif + ret = 0; + } + +cleanup: + if (ret < 0) + errno = saved_errno; + return ret; +} + +/* + * @argv argv to exec + * @envp optional environment to use for exec + * @keepfd options fd_ret to keep open for child process + * @retpid optional pointer to store child process pid + * @infd optional file descriptor to use as child input, otherwise /dev/null + * @outfd optional pointer to communicate output fd behavior + * outfd == NULL : Use /dev/null + * *outfd == -1 : Use a new fd + * *outfd != -1 : Use *outfd + * @errfd optional pointer to communcate error fd behavior. See outfd + * @flags possible combination of the following: + * VIR_EXEC_NONE : Default function behavior + * VIR_EXEC_NONBLOCK : Set child process output fd's as non-blocking + * VIR_EXEC_DAEMON : Daemonize the child process + * @hook optional virExecHook function to call prior to exec + * @data data to pass to the hook function + * @pidfile path to use as pidfile for daemonized process (needs DAEMON flag) + */ +static int +virExecWithHook(const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + pid_t *retpid, + int infd, int *outfd, int *errfd, + int flags, + virExecHook hook, + void *data, + char *pidfile) +{ + pid_t pid; + int null, i, openmax; + int pipeout[2] = {-1,-1}; + int pipeerr[2] = {-1,-1}; + int childout = -1; + int childerr = -1; + int tmpfd; + const char *binary = NULL; + int forkRet; + char *argv_str = NULL; + char *envp_str = NULL; + + if ((argv_str = virArgvToString(argv)) == NULL) { + virReportOOMError(); + return -1; + } + + if (envp) { + if ((envp_str = virArgvToString(envp)) == NULL) { + VIR_FREE(argv_str); + virReportOOMError(); + return -1; + } + VIR_DEBUG("%s %s", envp_str, argv_str); + VIR_FREE(envp_str); + } else { + VIR_DEBUG("%s", argv_str); + } + VIR_FREE(argv_str); + + if (argv[0][0] != '/') { + if (!(binary = virFindFileInPath(argv[0]))) { + virReportSystemError(ENOENT, + _("Cannot find '%s' in path"), + argv[0]); + return -1; + } + } else { + binary = argv[0]; + } + + if ((null = open("/dev/null", O_RDWR)) < 0) { + virReportSystemError(errno, + _("cannot open %s"), + "/dev/null"); + goto cleanup; + } + + if (outfd != NULL) { + if (*outfd == -1) { + if (pipe(pipeout) < 0) { + virReportSystemError(errno, + "%s", _("cannot create pipe")); + goto cleanup; + } + + if ((flags & VIR_EXEC_NONBLOCK) && + virSetNonBlock(pipeout[0]) == -1) { + virReportSystemError(errno, + "%s", _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + + if (virSetCloseExec(pipeout[0]) == -1) { + virReportSystemError(errno, + "%s", _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + + childout = pipeout[1]; + } else { + childout = *outfd; + } + } else { + childout = null; + } + + if (errfd != NULL) { + if (*errfd == -1) { + if (pipe(pipeerr) < 0) { + virReportSystemError(errno, + "%s", _("Failed to create pipe")); + goto cleanup; + } + + if ((flags & VIR_EXEC_NONBLOCK) && + virSetNonBlock(pipeerr[0]) == -1) { + virReportSystemError(errno, + "%s", _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + + if (virSetCloseExec(pipeerr[0]) == -1) { + virReportSystemError(errno, + "%s", _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + + childerr = pipeerr[1]; + } else { + childerr = *errfd; + } + } else { + childerr = null; + } + + forkRet = virFork(&pid); + + if (pid < 0) { + goto cleanup; + } + + if (pid) { /* parent */ + VIR_FORCE_CLOSE(null); + if (outfd && *outfd == -1) { + VIR_FORCE_CLOSE(pipeout[1]); + *outfd = pipeout[0]; + } + if (errfd && *errfd == -1) { + VIR_FORCE_CLOSE(pipeerr[1]); + *errfd = pipeerr[0]; + } + + if (forkRet < 0) { + goto cleanup; + } + + *retpid = pid; + + if (binary != argv[0]) + VIR_FREE(binary); + + return 0; + } + + /* child */ + + if (forkRet < 0) { + /* The fork was successful, but after that there was an error + * in the child (which was already logged). + */ + goto fork_error; + } + + openmax = sysconf (_SC_OPEN_MAX); + for (i = 3; i < openmax; i++) + if (i != infd && + i != null && + i != childout && + i != childerr && + (!keepfd || i >= FD_SETSIZE || !FD_ISSET(i, keepfd))) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + } + + if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) { + virReportSystemError(errno, + "%s", _("failed to setup stdin file handle")); + goto fork_error; + } + if (childout > 0 && + dup2(childout, STDOUT_FILENO) < 0) { + virReportSystemError(errno, + "%s", _("failed to setup stdout file handle")); + goto fork_error; + } + if (childerr > 0 && + dup2(childerr, STDERR_FILENO) < 0) { + virReportSystemError(errno, + "%s", _("failed to setup stderr file handle")); + goto fork_error; + } + + if (infd != STDIN_FILENO) + VIR_FORCE_CLOSE(infd); + VIR_FORCE_CLOSE(null); + if (childout > STDERR_FILENO) { + tmpfd = childout; /* preserve childout value */ + VIR_FORCE_CLOSE(tmpfd); + } + if (childerr > STDERR_FILENO && + childerr != childout) { + VIR_FORCE_CLOSE(childerr); + } + + /* Initialize full logging for a while */ + virLogSetFromEnv(); + + /* Daemonize as late as possible, so the parent process can detect + * the above errors with wait* */ + if (flags & VIR_EXEC_DAEMON) { + if (setsid() < 0) { + virReportSystemError(errno, + "%s", _("cannot become session leader")); + goto fork_error; + } + + if (chdir("/") < 0) { + virReportSystemError(errno, + "%s", _("cannot change to root directory")); + goto fork_error; + } + + pid = fork(); + if (pid < 0) { + virReportSystemError(errno, + "%s", _("cannot fork child process")); + goto fork_error; + } + + if (pid > 0) { + if (pidfile && virFileWritePidPath(pidfile,pid)) { + kill(pid, SIGTERM); + usleep(500*1000); + kill(pid, SIGTERM); + virReportSystemError(errno, + _("could not write pidfile %s for %d"), + pidfile, pid); + goto fork_error; + } + _exit(0); + } + } + + if (hook) { + /* virFork reset all signal handlers to the defaults. + * This is good for the child process, but our hook + * risks running something that generates SIGPIPE, + * so we need to temporarily block that again + */ + struct sigaction waxon, waxoff; + waxoff.sa_handler = SIG_IGN; + waxoff.sa_flags = 0; + sigemptyset(&waxoff.sa_mask); + memset(&waxon, 0, sizeof(waxon)); + if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) { + virReportSystemError(errno, "%s", + _("Could not disable SIGPIPE")); + goto fork_error; + } + + if ((hook)(data) != 0) { + VIR_DEBUG("Hook function failed."); + goto fork_error; + } + + if (sigaction(SIGPIPE, &waxon, NULL) < 0) { + virReportSystemError(errno, "%s", + _("Could not re-enable SIGPIPE")); + goto fork_error; + } + } + + /* The steps above may need todo something privileged, so + * we delay clearing capabilities until the last minute */ + if ((flags & VIR_EXEC_CLEAR_CAPS) && + virClearCapabilities() < 0) + goto fork_error; + + /* Close logging again to ensure no FDs leak to child */ + virLogReset(); + + if (envp) + execve(binary, (char **) argv, (char**)envp); + else + execv(binary, (char **) argv); + + virReportSystemError(errno, + _("cannot execute binary %s"), + argv[0]); + + fork_error: + virDispatchError(NULL); + _exit(EXIT_FAILURE); + + cleanup: + /* This is cleanup of parent process only - child + should never jump here on error */ + + if (binary != argv[0]) + VIR_FREE(binary); + + /* NB we don't virCommandError() on any failures here + because the code which jumped hre already raised + an error condition which we must not overwrite */ + VIR_FORCE_CLOSE(pipeerr[0]); + VIR_FORCE_CLOSE(pipeerr[1]); + VIR_FORCE_CLOSE(pipeout[0]); + VIR_FORCE_CLOSE(pipeout[1]); + VIR_FORCE_CLOSE(null); + return -1; +} + +/** + * @argv NULL terminated argv to run + * @status optional variable to return exit status in + * + * Run a command without using the shell. + * + * If status is NULL, then return 0 if the command run and + * exited with 0 status; Otherwise return -1 + * + * If status is not-NULL, then return 0 if the command ran. + * The status variable is filled with the command exit status + * and should be checked by caller for success. Return -1 + * only if the command could not be run. + */ +int +virRun(const char *const*argv, int *status) +{ + int ret; + virCommandPtr cmd = virCommandNewArgs(argv); + + ret = virCommandRun(cmd, status); + virCommandFree(cmd); + return ret; +} + +#else /* WIN32 */ + +virRun(const char *const *argv ATTRIBUTE_UNUSED, + int *status) +{ + if (status) + *status = ENOTSUP; + else + virCommandError(VIR_ERR_INTERNAL_ERROR, + "%s", _("virRun is not implemented for WIN32")); + return -1; +} + +static int +virExecWithHook(const char *const*argv ATTRIBUTE_UNUSED, + const char *const*envp ATTRIBUTE_UNUSED, + const fd_set *keepfd ATTRIBUTE_UNUSED, + pid_t *retpid ATTRIBUTE_UNUSED, + int infd ATTRIBUTE_UNUSED, + int *outfd ATTRIBUTE_UNUSED, + int *errfd ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED, + virExecHook hook ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + char *pidfile ATTRIBUTE_UNUSED) +{ + /* XXX: Some day we can implement pieces of virCommand/virExec on + * top of _spawn() or CreateProcess(), but we can't implement + * everything, since mingw completely lacks fork(), so we cannot + * run hook code in the child. */ + virCommandError(VIR_ERR_INTERNAL_ERROR, + "%s", _("virExec is not implemented for WIN32")); + return -1; +} + +int +virFork(pid_t *pid) +{ + *pid = -1; + errno = ENOTSUP; + + return -1; +} + +#endif /* WIN32 */ + + /* * Create a new command for named binary */ diff --git a/src/util/command.h b/src/util/command.h index aa5136b..267cd73 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -29,6 +29,20 @@ typedef struct _virCommand virCommand; typedef virCommand *virCommandPtr; +/* This will execute in the context of the first child + * after fork() but before execve() */ +typedef int (*virExecHook)(void *data); + +/* + * Fork wrapper with extra error checking + */ +int virFork(pid_t *pid) ATTRIBUTE_RETURN_CHECK; + +/* + * Simple synchronous command wrapper + */ +int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; + /* * Create a new command for named binary */ diff --git a/src/util/ebtables.c b/src/util/ebtables.c index 27dce5d..d1afff0 100644 --- a/src/util/ebtables.c +++ b/src/util/ebtables.c @@ -41,7 +41,7 @@ #include "internal.h" #include "ebtables.h" -#include "util.h" +#include "command.h" #include "memory.h" #include "virterror_internal.h" #include "logging.h" diff --git a/src/util/pci.c b/src/util/pci.c index 9cc7b20..51c3988 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -35,7 +35,7 @@ #include "logging.h" #include "memory.h" -#include "util.h" +#include "command.h" #include "virterror_internal.h" #include "files.h" diff --git a/src/util/util.c b/src/util/util.c index 92b31e2..f27f4e0 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -34,8 +34,8 @@ #include <errno.h> #include <poll.h> #include <time.h> -#include <sys/types.h> #include <sys/stat.h> +#include <sys/types.h> #include <sys/ioctl.h> #include <sys/wait.h> #include <sys/time.h> @@ -69,7 +69,6 @@ #include "virterror_internal.h" #include "logging.h" #include "event.h" -#include "ignore-value.h" #include "buf.h" #include "util.h" #include "memory.h" @@ -247,20 +246,6 @@ virArgvToString(const char *const *argv) return ret; } -int virSetBlocking(int fd, bool blocking) { - return set_nonblocking_flag (fd, !blocking); -} - -int virSetNonBlock(int fd) { - return virSetBlocking(fd, false); -} - - -int virSetCloseExec(int fd) -{ - return virSetInherit(fd, false); -} - #ifndef WIN32 int virSetInherit(int fd, bool inherit) { @@ -276,507 +261,6 @@ int virSetInherit(int fd, bool inherit) { return 0; } - -# if HAVE_CAPNG -static int virClearCapabilities(void) -{ - int ret; - - capng_clear(CAPNG_SELECT_BOTH); - - if ((ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) { - virUtilError(VIR_ERR_INTERNAL_ERROR, - _("cannot clear process capabilities %d"), ret); - return -1; - } - - return 0; -} -# else -static int virClearCapabilities(void) -{ -// VIR_WARN("libcap-ng support not compiled in, unable to clear capabilities"); - return 0; -} -# endif - - -/* virFork() - fork a new process while avoiding various race/deadlock conditions - - @pid - a pointer to a pid_t that will receive the return value from - fork() - - on return from virFork(), if *pid < 0, the fork failed and there is - no new process. Otherwise, just like fork(), if *pid == 0, it is the - child process returning, and if *pid > 0, it is the parent. - - Even if *pid >= 0, if the return value from virFork() is < 0, it - indicates a failure that occurred in the parent or child process - after the fork. In this case, the child process should call - _exit(EXIT_FAILURE) after doing any additional error reporting. - - */ -int virFork(pid_t *pid) { -# ifdef HAVE_PTHREAD_SIGMASK - sigset_t oldmask, newmask; -# endif - struct sigaction sig_action; - int saved_errno, ret = -1; - - *pid = -1; - - /* - * Need to block signals now, so that child process can safely - * kill off caller's signal handlers without a race. - */ -# ifdef HAVE_PTHREAD_SIGMASK - sigfillset(&newmask); - if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) { - saved_errno = errno; - virReportSystemError(errno, - "%s", _("cannot block signals")); - goto cleanup; - } -# endif - - /* Ensure we hold the logging lock, to protect child processes - * from deadlocking on another thread's inherited mutex state */ - virLogLock(); - - *pid = fork(); - saved_errno = errno; /* save for caller */ - - /* Unlock for both parent and child process */ - virLogUnlock(); - - if (*pid < 0) { -# ifdef HAVE_PTHREAD_SIGMASK - /* attempt to restore signal mask, but ignore failure, to - avoid obscuring the fork failure */ - ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); -# endif - virReportSystemError(saved_errno, - "%s", _("cannot fork child process")); - goto cleanup; - } - - if (*pid) { - - /* parent process */ - -# ifdef HAVE_PTHREAD_SIGMASK - /* Restore our original signal mask now that the child is - safely running */ - if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) { - saved_errno = errno; /* save for caller */ - virReportSystemError(errno, "%s", _("cannot unblock signals")); - goto cleanup; - } -# endif - ret = 0; - - } else { - - /* child process */ - - int logprio; - int i; - - /* Remove any error callback so errors in child now - get sent to stderr where they stand a fighting chance - of being seen / logged */ - virSetErrorFunc(NULL, NULL); - virSetErrorLogPriorityFunc(NULL); - - /* Make sure any hook logging is sent to stderr, since child - * process may close the logfile FDs */ - logprio = virLogGetDefaultPriority(); - virLogReset(); - virLogSetDefaultPriority(logprio); - - /* 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); - } - -# ifdef HAVE_PTHREAD_SIGMASK - /* 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) { - saved_errno = errno; /* save for caller */ - virReportSystemError(errno, "%s", _("cannot unblock signals")); - goto cleanup; - } -# endif - ret = 0; - } - -cleanup: - if (ret < 0) - errno = saved_errno; - return ret; -} - -/* - * @argv argv to exec - * @envp optional environment to use for exec - * @keepfd options fd_ret to keep open for child process - * @retpid optional pointer to store child process pid - * @infd optional file descriptor to use as child input, otherwise /dev/null - * @outfd optional pointer to communicate output fd behavior - * outfd == NULL : Use /dev/null - * *outfd == -1 : Use a new fd - * *outfd != -1 : Use *outfd - * @errfd optional pointer to communcate error fd behavior. See outfd - * @flags possible combination of the following: - * VIR_EXEC_NONE : Default function behavior - * VIR_EXEC_NONBLOCK : Set child process output fd's as non-blocking - * VIR_EXEC_DAEMON : Daemonize the child process - * @hook optional virExecHook function to call prior to exec - * @data data to pass to the hook function - * @pidfile path to use as pidfile for daemonized process (needs DAEMON flag) - */ -int -virExecWithHook(const char *const*argv, - const char *const*envp, - const fd_set *keepfd, - pid_t *retpid, - int infd, int *outfd, int *errfd, - int flags, - virExecHook hook, - void *data, - char *pidfile) -{ - pid_t pid; - int null, i, openmax; - int pipeout[2] = {-1,-1}; - int pipeerr[2] = {-1,-1}; - int childout = -1; - int childerr = -1; - int tmpfd; - const char *binary = NULL; - int forkRet; - char *argv_str = NULL; - char *envp_str = NULL; - - if ((argv_str = virArgvToString(argv)) == NULL) { - virReportOOMError(); - return -1; - } - - if (envp) { - if ((envp_str = virArgvToString(envp)) == NULL) { - VIR_FREE(argv_str); - virReportOOMError(); - return -1; - } - VIR_DEBUG("%s %s", envp_str, argv_str); - VIR_FREE(envp_str); - } else { - VIR_DEBUG("%s", argv_str); - } - VIR_FREE(argv_str); - - if (argv[0][0] != '/') { - if (!(binary = virFindFileInPath(argv[0]))) { - virReportSystemError(ENOENT, - _("Cannot find '%s' in path"), - argv[0]); - return -1; - } - } else { - binary = argv[0]; - } - - if ((null = open("/dev/null", O_RDWR)) < 0) { - virReportSystemError(errno, - _("cannot open %s"), - "/dev/null"); - goto cleanup; - } - - if (outfd != NULL) { - if (*outfd == -1) { - if (pipe(pipeout) < 0) { - virReportSystemError(errno, - "%s", _("cannot create pipe")); - goto cleanup; - } - - if ((flags & VIR_EXEC_NONBLOCK) && - virSetNonBlock(pipeout[0]) == -1) { - virReportSystemError(errno, - "%s", _("Failed to set non-blocking file descriptor flag")); - goto cleanup; - } - - if (virSetCloseExec(pipeout[0]) == -1) { - virReportSystemError(errno, - "%s", _("Failed to set close-on-exec file descriptor flag")); - goto cleanup; - } - - childout = pipeout[1]; - } else { - childout = *outfd; - } - } else { - childout = null; - } - - if (errfd != NULL) { - if (*errfd == -1) { - if (pipe(pipeerr) < 0) { - virReportSystemError(errno, - "%s", _("Failed to create pipe")); - goto cleanup; - } - - if ((flags & VIR_EXEC_NONBLOCK) && - virSetNonBlock(pipeerr[0]) == -1) { - virReportSystemError(errno, - "%s", _("Failed to set non-blocking file descriptor flag")); - goto cleanup; - } - - if (virSetCloseExec(pipeerr[0]) == -1) { - virReportSystemError(errno, - "%s", _("Failed to set close-on-exec file descriptor flag")); - goto cleanup; - } - - childerr = pipeerr[1]; - } else { - childerr = *errfd; - } - } else { - childerr = null; - } - - forkRet = virFork(&pid); - - if (pid < 0) { - goto cleanup; - } - - if (pid) { /* parent */ - VIR_FORCE_CLOSE(null); - if (outfd && *outfd == -1) { - VIR_FORCE_CLOSE(pipeout[1]); - *outfd = pipeout[0]; - } - if (errfd && *errfd == -1) { - VIR_FORCE_CLOSE(pipeerr[1]); - *errfd = pipeerr[0]; - } - - if (forkRet < 0) { - goto cleanup; - } - - *retpid = pid; - - if (binary != argv[0]) - VIR_FREE(binary); - - return 0; - } - - /* child */ - - if (forkRet < 0) { - /* The fork was sucessful, but after that there was an error - * in the child (which was already logged). - */ - goto fork_error; - } - - openmax = sysconf (_SC_OPEN_MAX); - for (i = 3; i < openmax; i++) - if (i != infd && - i != null && - i != childout && - i != childerr && - (!keepfd || i >= FD_SETSIZE || !FD_ISSET(i, keepfd))) { - tmpfd = i; - VIR_FORCE_CLOSE(tmpfd); - } - - if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) { - virReportSystemError(errno, - "%s", _("failed to setup stdin file handle")); - goto fork_error; - } - if (childout > 0 && - dup2(childout, STDOUT_FILENO) < 0) { - virReportSystemError(errno, - "%s", _("failed to setup stdout file handle")); - goto fork_error; - } - if (childerr > 0 && - dup2(childerr, STDERR_FILENO) < 0) { - virReportSystemError(errno, - "%s", _("failed to setup stderr file handle")); - goto fork_error; - } - - if (infd != STDIN_FILENO) - VIR_FORCE_CLOSE(infd); - VIR_FORCE_CLOSE(null); - if (childout > STDERR_FILENO) { - tmpfd = childout; /* preserve childout value */ - VIR_FORCE_CLOSE(tmpfd); - } - if (childerr > STDERR_FILENO && - childerr != childout) { - VIR_FORCE_CLOSE(childerr); - } - - /* Initialize full logging for a while */ - virLogSetFromEnv(); - - /* Daemonize as late as possible, so the parent process can detect - * the above errors with wait* */ - if (flags & VIR_EXEC_DAEMON) { - if (setsid() < 0) { - virReportSystemError(errno, - "%s", _("cannot become session leader")); - goto fork_error; - } - - if (chdir("/") < 0) { - virReportSystemError(errno, - "%s", _("cannot change to root directory")); - goto fork_error; - } - - pid = fork(); - if (pid < 0) { - virReportSystemError(errno, - "%s", _("cannot fork child process")); - goto fork_error; - } - - if (pid > 0) { - if (pidfile && virFileWritePidPath(pidfile,pid)) { - kill(pid, SIGTERM); - usleep(500*1000); - kill(pid, SIGTERM); - virReportSystemError(errno, - _("could not write pidfile %s for %d"), - pidfile, pid); - goto fork_error; - } - _exit(0); - } - } - - if (hook) { - /* virFork reset all signal handlers to the defaults. - * This is good for the child process, but our hook - * risks running something that generates SIGPIPE, - * so we need to temporarily block that again - */ - struct sigaction waxon, waxoff; - waxoff.sa_handler = SIG_IGN; - waxoff.sa_flags = 0; - sigemptyset(&waxoff.sa_mask); - memset(&waxon, 0, sizeof(waxon)); - if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) { - virReportSystemError(errno, "%s", - _("Could not disable SIGPIPE")); - goto fork_error; - } - - if ((hook)(data) != 0) { - VIR_DEBUG("Hook function failed."); - goto fork_error; - } - - if (sigaction(SIGPIPE, &waxon, NULL) < 0) { - virReportSystemError(errno, "%s", - _("Could not re-enable SIGPIPE")); - goto fork_error; - } - } - - /* The steps above may need todo something privileged, so - * we delay clearing capabilities until the last minute */ - if ((flags & VIR_EXEC_CLEAR_CAPS) && - virClearCapabilities() < 0) - goto fork_error; - - /* Close logging again to ensure no FDs leak to child */ - virLogReset(); - - if (envp) - execve(binary, (char **) argv, (char**)envp); - else - execv(binary, (char **) argv); - - virReportSystemError(errno, - _("cannot execute binary %s"), - argv[0]); - - fork_error: - virDispatchError(NULL); - _exit(EXIT_FAILURE); - - cleanup: - /* This is cleanup of parent process only - child - should never jump here on error */ - - if (binary != argv[0]) - VIR_FREE(binary); - - /* NB we don't virUtilError() on any failures here - because the code which jumped hre already raised - an error condition which we must not overwrite */ - VIR_FORCE_CLOSE(pipeerr[0]); - VIR_FORCE_CLOSE(pipeerr[1]); - VIR_FORCE_CLOSE(pipeout[0]); - VIR_FORCE_CLOSE(pipeout[1]); - VIR_FORCE_CLOSE(null); - return -1; -} - -/** - * @argv NULL terminated argv to run - * @status optional variable to return exit status in - * - * Run a command without using the shell. - * - * If status is NULL, then return 0 if the command run and - * exited with 0 status; Otherwise return -1 - * - * If status is not-NULL, then return 0 if the command ran. - * The status variable is filled with the command exit status - * and should be checked by caller for success. Return -1 - * only if the command could not be run. - */ -int -virRun(const char *const*argv, int *status) -{ - int ret; - virCommandPtr cmd = virCommandNewArgs(argv); - - ret = virCommandRun(cmd, status); - virCommandFree(cmd); - return ret; -} - #else /* WIN32 */ int virSetInherit(int fd ATTRIBUTE_UNUSED, bool inherit ATTRIBUTE_UNUSED) @@ -784,50 +268,21 @@ int virSetInherit(int fd ATTRIBUTE_UNUSED, bool inherit ATTRIBUTE_UNUSED) return -1; } -virRun(const char *const *argv ATTRIBUTE_UNUSED, - int *status) -{ - if (status) - *status = ENOTSUP; - else - virUtilError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virRun is not implemented for WIN32")); - return -1; +#endif /* WIN32 */ + +int virSetBlocking(int fd, bool blocking) { + return set_nonblocking_flag (fd, !blocking); } -int -virExecWithHook(const char *const*argv ATTRIBUTE_UNUSED, - const char *const*envp ATTRIBUTE_UNUSED, - const fd_set *keepfd ATTRIBUTE_UNUSED, - pid_t *retpid ATTRIBUTE_UNUSED, - int infd ATTRIBUTE_UNUSED, - int *outfd ATTRIBUTE_UNUSED, - int *errfd ATTRIBUTE_UNUSED, - int flags ATTRIBUTE_UNUSED, - virExecHook hook ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED, - char *pidfile ATTRIBUTE_UNUSED) -{ - /* XXX: Some day we can implement pieces of virCommand/virExec on - * top of _spawn() or CreateProcess(), but we can't implement - * everything, since mingw completely lacks fork(), so we cannot - * run hook code in the child. */ - virUtilError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virExec is not implemented for WIN32")); - return -1; +int virSetNonBlock(int fd) { + return virSetBlocking(fd, false); } -int -virFork(pid_t *pid) +int virSetCloseExec(int fd) { - *pid = -1; - errno = ENOTSUP; - - return -1; + return virSetInherit(fd, false); } -#endif /* WIN32 */ - int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf) { diff --git a/src/util/util.h b/src/util/util.h index 3e95cae..68a8431 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -42,37 +42,13 @@ ssize_t safewrite(int fd, const void *buf, size_t count) int safezero(int fd, int flags, off_t offset, off_t len) ATTRIBUTE_RETURN_CHECK; -enum { - VIR_EXEC_NONE = 0, - VIR_EXEC_NONBLOCK = (1 << 0), - VIR_EXEC_DAEMON = (1 << 1), - VIR_EXEC_CLEAR_CAPS = (1 << 2), -}; - int virSetBlocking(int fd, bool blocking) ATTRIBUTE_RETURN_CHECK; int virSetNonBlock(int fd) ATTRIBUTE_RETURN_CHECK; int virSetInherit(int fd, bool inherit) ATTRIBUTE_RETURN_CHECK; int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK; -/* This will execute in the context of the first child - * after fork() but before execve() */ -typedef int (*virExecHook)(void *data); - -int virExecWithHook(const char *const*argv, - const char *const*envp, - const fd_set *keepfd, - int *retpid, - int infd, - int *outfd, - int *errfd, - int flags, - virExecHook hook, - void *data, - char *pidfile) ATTRIBUTE_RETURN_CHECK; -int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf); -int virFork(pid_t *pid); int virSetUIDGID(uid_t uid, gid_t gid); diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index ab41d02..5e2c4ba 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -29,6 +29,7 @@ #include "files.h" #include "memory.h" #include "uuid.h" +#include "command.h" #include "vmx.h" #include "vmware_conf.h" #include "vmware_driver.h" -- 1.7.4.4

On 05/17/2011 09:06 AM, Cole Robinson wrote:
Seems reasonable to have all command wrappers in the same place
v2: Dont move SetInherit
v3: Comment spelling fix Adjust WARN0 comment Remove spurious #include movement Don't include sys/types.h Combine virExec enums
Signed-off-by: Cole Robinson <crobinso@redhat.com> @@ -42,9 +48,13 @@ virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__)
Looks like you addressed all my comments; and sorry for the delay on this last round of reviews. ACK. Overall, this is a rather large change to be pushing after feature freeze; I'm 50-50 on whether we should delay this until after the release, since it is missing test exposure by not being part of RC1. Does anyone else have an opinion on whether this is safe enough to push now? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 06/01/2011 03:52 PM, Eric Blake wrote:
On 05/17/2011 09:06 AM, Cole Robinson wrote:
Seems reasonable to have all command wrappers in the same place
v2: Dont move SetInherit
v3: Comment spelling fix Adjust WARN0 comment Remove spurious #include movement Don't include sys/types.h Combine virExec enums
Signed-off-by: Cole Robinson <crobinso@redhat.com> @@ -42,9 +48,13 @@ virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__)
Looks like you addressed all my comments; and sorry for the delay on this last round of reviews.
ACK.
Overall, this is a rather large change to be pushing after feature freeze; I'm 50-50 on whether we should delay this until after the release, since it is missing test exposure by not being part of RC1. Does anyone else have an opinion on whether this is safe enough to push now?
Not really worth potentially rocking the boat over, I say we just wait till after the release. Thanks, Cole

On 06/01/2011 04:02 PM, Cole Robinson wrote:
On 06/01/2011 03:52 PM, Eric Blake wrote:
On 05/17/2011 09:06 AM, Cole Robinson wrote:
Seems reasonable to have all command wrappers in the same place
v2: Dont move SetInherit
v3: Comment spelling fix Adjust WARN0 comment Remove spurious #include movement Don't include sys/types.h Combine virExec enums
Signed-off-by: Cole Robinson <crobinso@redhat.com> @@ -42,9 +48,13 @@ virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__)
Looks like you addressed all my comments; and sorry for the delay on this last round of reviews.
ACK.
Overall, this is a rather large change to be pushing after feature freeze; I'm 50-50 on whether we should delay this until after the release, since it is missing test exposure by not being part of RC1. Does anyone else have an opinion on whether this is safe enough to push now?
Not really worth potentially rocking the boat over, I say we just wait till after the release.
I've pushed this series now. Thanks, Cole
participants (2)
-
Cole Robinson
-
Eric Blake