[libvirt] [PATCH 0/9 v2] More virCommand conversions and cleanups
The following series converts all users of several older command wrappers (virRunWithHook, virExecDaemonize, and 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 Cole Robinson (9): storage: iscsi: Convert virExec to virCommand openvz: Convert virExec usage to virCommand qemu: Convert virExec usage to virCommand storage: Covert regex helpers to virCommand storage_backend: Fix error reporting with regex helper storage_backend: Convert virRunWithHook 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 | 2 +- src/libvirt_private.syms | 7 +- src/lxc/veth.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 1 + src/openvz/openvz_conf.c | 37 +- src/openvz/openvz_driver.c | 55 ++-- src/qemu/qemu_driver.c | 35 +- src/qemu/qemu_process.c | 2 +- src/storage/storage_backend.c | 262 ++++-------- src/storage/storage_backend.h | 3 +- src/storage/storage_backend_fs.c | 7 +- src/storage/storage_backend_iscsi.c | 39 +-- src/storage/storage_backend_logical.c | 32 +-- src/util/command.c | 568 ++++++++++++++++++++++++ src/util/command.h | 14 + src/util/ebtables.c | 2 +- src/util/pci.c | 2 +- src/util/util.c | 674 +---------------------------- src/util/util.h | 35 -- src/vmware/vmware_driver.c | 1 + 20 files changed, 765 insertions(+), 1015 deletions(-) -- 1.7.4.4
v2: Use virCommand auto-cleanup Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage/storage_backend_iscsi.c | 23 +++++++++++------------ 1 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ec52dbc..bdd58cd 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -184,28 +184,23 @@ virStorageBackendIQNFound(const char *initiatoriqn, int ret = IQN_MISSING, fd = -1; char ebuf[64]; FILE *fp = NULL; - pid_t child = 0; char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL, *saveptr = NULL; - const char *const prog[] = { - ISCSIADM, "--mode", "iface", NULL - }; + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, + "--mode", "iface", NULL); if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { ret = IQN_ERROR; virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Could not allocate memory for output of '%s'"), - prog[0]); + ISCSIADM); goto out; } memset(line, 0, LINE_SIZE); - if (virExec(prog, NULL, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run '%s' when looking for existing interface with IQN '%s'"), - prog[0], initiatoriqn); - + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { ret = IQN_ERROR; goto out; } @@ -214,7 +209,7 @@ virStorageBackendIQNFound(const char *initiatoriqn, virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open stream for file descriptor " "when reading output from '%s': '%s'"), - prog[0], virStrerror(errno, ebuf, sizeof ebuf)); + ISCSIADM, virStrerror(errno, ebuf, sizeof ebuf)); ret = IQN_ERROR; goto out; } @@ -226,7 +221,7 @@ virStorageBackendIQNFound(const char *initiatoriqn, virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected line > %d characters " "when parsing output of '%s'"), - LINE_SIZE, prog[0]); + LINE_SIZE, ISCSIADM); goto out; } *newline = '\0'; @@ -251,6 +246,9 @@ virStorageBackendIQNFound(const char *initiatoriqn, } } + if (virCommandWait(cmd, NULL) < 0) + ret = IQN_ERROR; + out: if (ret == IQN_MISSING) { VIR_DEBUG("Could not find interface with IQN '%s'", iqn); @@ -259,6 +257,7 @@ out: VIR_FREE(line); VIR_FORCE_FCLOSE(fp); VIR_FORCE_CLOSE(fd); + virCommandFree(cmd); return ret; } -- 1.7.4.4
On 05/13/2011 02:10 PM, Cole Robinson wrote:
v2: Use virCommand auto-cleanup
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage/storage_backend_iscsi.c | 23 +++++++++++------------ 1 files changed, 11 insertions(+), 12 deletions(-)
memset(line, 0, LINE_SIZE);
- if (virExec(prog, NULL, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run '%s' when looking for existing interface with IQN '%s'"), - prog[0], initiatoriqn); - + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { ret = IQN_ERROR;
...
+ if (virCommandWait(cmd, NULL) < 0) + ret = IQN_ERROR; + out: if (ret == IQN_MISSING) { VIR_DEBUG("Could not find interface with IQN '%s'", iqn); @@ -259,6 +257,7 @@ out: VIR_FREE(line); VIR_FORCE_FCLOSE(fp); VIR_FORCE_CLOSE(fd); + virCommandFree(cmd);
Yep, that does the trick for auto-cleanup (I'm liking virCommand more and more as we convert things to use it). ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
v2: Use virCommand's autocleanup Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/openvz/openvz_conf.c | 37 +++++++++-------------------- src/openvz/openvz_driver.c | 55 +++++++++++++++++++++++-------------------- 2 files changed, 41 insertions(+), 51 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 45bc398..d038119 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -74,53 +74,40 @@ strtoI(const char *str) static int -openvzExtractVersionInfo(const char *cmd, int *retversion) +openvzExtractVersionInfo(const char *cmdstr, int *retversion) { - const char *const vzarg[] = { cmd, "--help", NULL }; - const char *const vzenv[] = { "LC_ALL=C", NULL }; - pid_t child; - int newstdout = -1; - int ret = -1, status; + int ret = -1; unsigned long version; + char *help = NULL; char *tmp; + virCommandPtr cmd = virCommandNewArgList(cmdstr, "--help", NULL); if (retversion) *retversion = 0; - if (virExec(vzarg, vzenv, NULL, - &child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) - return -1; + virCommandAddEnvString(cmd, "LC_ALL=C"); + virCommandSetOutputBuffer(cmd, &help); - char *help = NULL; - int len = virFileReadLimFD(newstdout, 4096, &help); - if (len < 0) - goto cleanup2; + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; tmp = help; /* expected format: vzctl version <major>.<minor>.<micro> */ if ((tmp = STRSKIP(tmp, "vzctl version ")) == NULL) - goto cleanup2; + goto cleanup; if (virParseVersionString(tmp, &version) < 0) - goto cleanup2; + goto cleanup; if (retversion) *retversion = version; ret = 0; -cleanup2: +cleanup: + virCommandFree(cmd); VIR_FREE(help); - if (VIR_CLOSE(newstdout) < 0) - ret = -1; - -rewait: - if (waitpid(child, &status, 0) != child) { - if (errno == EINTR) - goto rewait; - ret = -1; - } return ret; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 3e1952f..7d43272 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -41,7 +41,6 @@ #include <sys/utsname.h> #include <sys/stat.h> #include <fcntl.h> -#include <signal.h> #include <paths.h> #include <pwd.h> #include <stdio.h> @@ -59,6 +58,7 @@ #include "bridge.h" #include "files.h" #include "logging.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -1363,21 +1363,16 @@ static int openvzListDomains(virConnectPtr conn ATTRIBUTE_UNUSED, int *ids, int nids) { int got = 0; int veid; - pid_t pid; int outfd = -1; + int rc = -1; int ret; char buf[32]; char *endptr; - const char *cmd[] = {VZLIST, "-ovpsid", "-H" , NULL}; - - ret = virExec(cmd, NULL, NULL, - &pid, -1, &outfd, NULL, VIR_EXEC_NONE); - if (ret == -1) { - openvzError(VIR_ERR_INTERNAL_ERROR, - _("Could not exec %s"), VZLIST); - VIR_FORCE_CLOSE(outfd); - return -1; - } + virCommandPtr cmd = virCommandNewArgList(VZLIST, "-ovpsid", "-H" , NULL); + + virCommandSetOutputFD(cmd, &outfd); + if (virCommandRunAsync(cmd, NULL) < 0) + goto cleanup; while (got < nids) { ret = openvz_readline(outfd, buf, 32); @@ -1391,13 +1386,20 @@ static int openvzListDomains(virConnectPtr conn ATTRIBUTE_UNUSED, ids[got] = veid; got ++; } - waitpid(pid, NULL, 0); + + if (virCommandWait(cmd, NULL) < 0) + goto cleanup; if (VIR_CLOSE(outfd) < 0) { virReportSystemError(errno, "%s", _("failed to close file")); - return -1; + goto cleanup; } - return got; + + rc = got; +cleanup: + VIR_FORCE_CLOSE(outfd); + virCommandFree(cmd); + return rc; } static int openvzNumDomains(virConnectPtr conn) { @@ -1415,20 +1417,17 @@ static int openvzListDefinedDomains(virConnectPtr conn ATTRIBUTE_UNUSED, char **const names, int nnames) { int got = 0; int veid, outfd = -1, ret; - pid_t pid; + int rc = -1; char vpsname[32]; char buf[32]; char *endptr; - const char *cmd[] = {VZLIST, "-ovpsid", "-H", "-S", NULL}; + virCommandPtr cmd = virCommandNewArgList(VZLIST, + "-ovpsid", "-H", "-S", NULL); /* the -S options lists only stopped domains */ - ret = virExec(cmd, NULL, NULL, - &pid, -1, &outfd, NULL, VIR_EXEC_NONE); - if (ret == -1) { - openvzError(VIR_ERR_INTERNAL_ERROR, - _("Could not exec %s"), VZLIST); + virCommandSetOutputFD(cmd, &outfd); + if (virCommandRunAsync(cmd, NULL) < 0) goto out; - } while (got < nnames) { ret = openvz_readline(outfd, buf, 32); @@ -1446,18 +1445,22 @@ static int openvzListDefinedDomains(virConnectPtr conn ATTRIBUTE_UNUSED, } got ++; } - waitpid(pid, NULL, 0); + + if (virCommandWait(cmd, NULL) < 0) + goto out; + if (VIR_CLOSE(outfd) < 0) { virReportSystemError(errno, "%s", _("failed to close file")); goto out; } - return got; + rc = got; out: VIR_FORCE_CLOSE(outfd); + virCommandFree(cmd); for ( ; got >= 0 ; got--) VIR_FREE(names[got]); - return -1; + return rc; } static int openvzGetProcessInfo(unsigned long long *cpuTime, int vpsid) -- 1.7.4.4
On 05/13/2011 02:10 PM, Cole Robinson wrote:
v2: Use virCommand's autocleanup
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/openvz/openvz_conf.c | 37 +++++++++-------------------- src/openvz/openvz_driver.c | 55 +++++++++++++++++++++++-------------------- 2 files changed, 41 insertions(+), 51 deletions(-)
@@ -1446,18 +1445,22 @@ static int openvzListDefinedDomains(virConnectPtr conn ATTRIBUTE_UNUSED, } got ++; } - waitpid(pid, NULL, 0); + + if (virCommandWait(cmd, NULL) < 0) + goto out; + if (VIR_CLOSE(outfd) < 0) { virReportSystemError(errno, "%s", _("failed to close file")); goto out; } - return got;
+ rc = got; out: VIR_FORCE_CLOSE(outfd); + virCommandFree(cmd); for ( ; got >= 0 ; got--) VIR_FREE(names[got]);
Oops, this now frees names[] on success. You need to gate this for loop on whether rc >= 0, now that the success path falls through to the out label. ACK with that nit fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/13/2011 04:47 PM, Eric Blake wrote:
On 05/13/2011 02:10 PM, Cole Robinson wrote:
v2: Use virCommand's autocleanup
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/openvz/openvz_conf.c | 37 +++++++++-------------------- src/openvz/openvz_driver.c | 55 +++++++++++++++++++++++-------------------- 2 files changed, 41 insertions(+), 51 deletions(-)
@@ -1446,18 +1445,22 @@ static int openvzListDefinedDomains(virConnectPtr conn ATTRIBUTE_UNUSED, } got ++; } - waitpid(pid, NULL, 0); + + if (virCommandWait(cmd, NULL) < 0) + goto out; + if (VIR_CLOSE(outfd) < 0) { virReportSystemError(errno, "%s", _("failed to close file")); goto out; } - return got;
+ rc = got; out: VIR_FORCE_CLOSE(outfd); + virCommandFree(cmd); for ( ; got >= 0 ; got--) VIR_FREE(names[got]);
Oops, this now frees names[] on success. You need to gate this for loop on whether rc >= 0, now that the success path falls through to the out label.
ACK with that nit fixed.
Thanks, pushed with that fix. - Cole
On 05/17/2011 08:24 AM, Cole Robinson wrote:
+ rc = got; out: VIR_FORCE_CLOSE(outfd); + virCommandFree(cmd); for ( ; got >= 0 ; got--) VIR_FREE(names[got]);
Oops, this now frees names[] on success. You need to gate this for loop on whether rc >= 0, now that the success path falls through to the out label.
ACK with that nit fixed.
Thanks, pushed with that fix.
Phooey, we got it backwards. On success, we want names[0] to be valid. On failure, we want names[0] to be NULL. So the condition needs to be if (rc < 0) { free names }. I'm pushing this followup under the trivial rule: diff --git i/src/openvz/openvz_driver.c w/src/openvz/openvz_driver.c index cefba16..ae951a2 100644 --- i/src/openvz/openvz_driver.c +++ w/src/openvz/openvz_driver.c @@ -1492,7 +1492,7 @@ static int openvzListDefinedDomains(virConnectPtr conn ATTRIBUTE_UNUSED, out: VIR_FORCE_CLOSE(outfd); virCommandFree(cmd); - if (rc >= 0) { + if (rc < 0) { for ( ; got >= 0 ; got--) VIR_FREE(names[got]); } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/17/2011 11:12 AM, Eric Blake wrote:
On 05/17/2011 08:24 AM, Cole Robinson wrote:
+ rc = got; out: VIR_FORCE_CLOSE(outfd); + virCommandFree(cmd); for ( ; got >= 0 ; got--) VIR_FREE(names[got]);
Oops, this now frees names[] on success. You need to gate this for loop on whether rc >= 0, now that the success path falls through to the out label.
ACK with that nit fixed.
Thanks, pushed with that fix.
Phooey, we got it backwards. On success, we want names[0] to be valid. On failure, we want names[0] to be NULL.
So the condition needs to be if (rc < 0) { free names }.
I'm pushing this followup under the trivial rule:
diff --git i/src/openvz/openvz_driver.c w/src/openvz/openvz_driver.c index cefba16..ae951a2 100644 --- i/src/openvz/openvz_driver.c +++ w/src/openvz/openvz_driver.c @@ -1492,7 +1492,7 @@ static int openvzListDefinedDomains(virConnectPtr conn ATTRIBUTE_UNUSED, out: VIR_FORCE_CLOSE(outfd); virCommandFree(cmd); - if (rc >= 0) { + if (rc < 0) { for ( ; got >= 0 ; got--) VIR_FREE(names[got]); }
Urgh sorry, thanks for confirming the correct fix. - Cole
v2: Have virCommand cleanup intermediate process for us Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 35 +++++++++++------------------------ src/qemu/qemu_process.c | 2 +- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fff41e0..a826b6d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3238,11 +3238,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, @@ -3252,14 +3250,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; } @@ -3270,23 +3271,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, ret = qemuProcessStart(conn, driver, vm, "stdio", true, *fd, path, VIR_VM_OP_RESTORE); - if (intermediate_pid != -1) { - if (ret < 0) { - /* if there was an error setting up qemu, the intermediate - * process will wait forever to write to stdout, so we - * must manually kill it. - */ - 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 (cmd && virCommandWait(cmd, NULL) < 0) + goto out; VIR_FORCE_CLOSE(intermediatefd); if (VIR_CLOSE(*fd) < 0) { @@ -3324,6 +3310,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 f745aea..96c01f7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2275,7 +2275,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/13/2011 02:10 PM, Cole Robinson wrote:
v2: Have virCommand cleanup intermediate process for us
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 35 +++++++++++------------------------ src/qemu/qemu_process.c | 2 +- 2 files changed, 12 insertions(+), 25 deletions(-)
@@ -3270,23 +3271,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, ret = qemuProcessStart(conn, driver, vm, "stdio", true, *fd, path, VIR_VM_OP_RESTORE);
- if (intermediate_pid != -1) { - if (ret < 0) { - /* if there was an error setting up qemu, the intermediate - * process will wait forever to write to stdout, so we - * must manually kill it. - */ - 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 (cmd && virCommandWait(cmd, NULL) < 0) + goto out; VIR_FORCE_CLOSE(intermediatefd);
if (VIR_CLOSE(*fd) < 0) { @@ -3324,6 +3310,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, ret = 0;
out: + virCommandFree(cmd);
FD leak. Now you can get to the 'out' label without closing intermdiatefd. But we also have to be careful that we don't close things twice (since intermediatefd is sometimes set to *fd, but the caller may close *fd). I think we need a v3 just to make sure we get the fd semantics correct on this one. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
v2: Simplify command creation Add a missing virCommandFree Use virCommand auto-cleanup for async process Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage/storage_backend.c | 91 +++++++++------------------------------- 1 files changed, 21 insertions(+), 70 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e05679d..a78793a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1404,8 +1404,7 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, void *data, int *outexit) { - int fd = -1, exitstatus, err, failed = 1; - pid_t child = 0; + int fd = -1, err, ret = -1; FILE *list = NULL; regex_t *reg; regmatch_t *vars = NULL; @@ -1413,6 +1412,7 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, int maxReg = 0, i, j; int totgroups = 0, ngroup = 0, maxvars = 0; char **groups; + virCommandPtr cmd = NULL; /* Compile all regular expressions */ if (VIR_ALLOC_N(reg, nregex) < 0) { @@ -1449,10 +1449,9 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, goto cleanup; } - - /* Run the program and capture its output */ - if (virExec(prog, NULL, NULL, - &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { + cmd = virCommandNewArgs(prog); + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { goto cleanup; } @@ -1501,9 +1500,8 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, } } - failed = 0; - - cleanup: + ret = virCommandWait(cmd, outexit); +cleanup: if (groups) { for (j = 0 ; j < totgroups ; j++) VIR_FREE(groups[j]); @@ -1515,33 +1513,12 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, regfree(®[i]); VIR_FREE(reg); + virCommandFree(cmd); VIR_FORCE_FCLOSE(list); VIR_FORCE_CLOSE(fd); - while ((err = waitpid(child, &exitstatus, 0) == -1) && errno == EINTR); - - /* Don't bother checking exit status if we already failed */ - if (failed) - return -1; - - if (err == -1) { - virReportSystemError(errno, - _("failed to wait for command '%s'"), - prog[0]); - return -1; - } else { - if (WIFEXITED(exitstatus)) { - if (outexit != NULL) - *outexit = WEXITSTATUS(exitstatus); - } else { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("command did not exit cleanly")); - return -1; - } - } - - return 0; + return ret; } /* @@ -1563,13 +1540,12 @@ virStorageBackendRunProgNul(virStoragePoolObjPtr pool, void *data) { size_t n_tok = 0; - int fd = -1, exitstatus; - pid_t child = 0; + int fd = -1; FILE *fp = NULL; char **v; - int err = -1; - int w_err; + int ret = -1; int i; + virCommandPtr cmd = NULL; if (n_columns == 0) return -1; @@ -1581,9 +1557,9 @@ virStorageBackendRunProgNul(virStoragePoolObjPtr pool, for (i = 0; i < n_columns; i++) v[i] = NULL; - /* Run the program and capture its output */ - if (virExec(prog, NULL, NULL, - &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { + cmd = virCommandNewArgs(prog); + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { goto cleanup; } @@ -1618,48 +1594,23 @@ virStorageBackendRunProgNul(virStoragePoolObjPtr pool, } } - if (feof (fp)) - err = 0; - else + if (feof (fp) < 0) { virReportSystemError(errno, _("read error on pipe to '%s'"), prog[0]); + goto cleanup; + } + ret = virCommandWait(cmd, NULL); cleanup: for (i = 0; i < n_columns; i++) VIR_FREE(v[i]); VIR_FREE(v); + virCommandFree(cmd); VIR_FORCE_FCLOSE(fp); VIR_FORCE_CLOSE(fd); - while ((w_err = waitpid (child, &exitstatus, 0) == -1) && errno == EINTR) - /* empty */ ; - - /* Don't bother checking exit status if we already failed */ - if (err < 0) - return -1; - - if (w_err == -1) { - virReportSystemError(errno, - _("failed to wait for command '%s'"), - prog[0]); - return -1; - } else { - if (WIFEXITED(exitstatus)) { - if (WEXITSTATUS(exitstatus) != 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("non-zero exit status from command %d"), - WEXITSTATUS(exitstatus)); - return -1; - } - } else { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("command did not exit cleanly")); - return -1; - } - } - - return 0; + return ret; } #else /* WIN32 */ -- 1.7.4.4
On 05/13/2011 02:10 PM, Cole Robinson wrote:
v2: Simplify command creation Add a missing virCommandFree Use virCommand auto-cleanup for async process
Signed-off-by: Cole Robinson <crobinso@redhat.com> ---
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Some clients overwrite the error from the regex helper, or do half-baked error reporting with the exitstatus. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage/storage_backend.c | 11 +++++------ src/storage/storage_backend.h | 3 +-- src/storage/storage_backend_fs.c | 5 ++--- src/storage/storage_backend_iscsi.c | 16 ++-------------- src/storage/storage_backend_logical.c | 30 +++++------------------------- 5 files changed, 15 insertions(+), 50 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a78793a..61d14b1 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1401,8 +1401,7 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, const char **regex, int *nvars, virStorageBackendListVolRegexFunc func, - void *data, - int *outexit) + void *data) { int fd = -1, err, ret = -1; FILE *list = NULL; @@ -1500,7 +1499,7 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, } } - ret = virCommandWait(cmd, outexit); + ret = virCommandWait(cmd, NULL); cleanup: if (groups) { for (j = 0 ; j < totgroups ; j++) @@ -1623,10 +1622,10 @@ virStorageBackendRunProgRegex(virConnectPtr conn, const char **regex ATTRIBUTE_UNUSED, int *nvars ATTRIBUTE_UNUSED, virStorageBackendListVolRegexFunc func ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED, - int *outexit ATTRIBUTE_UNUSED) + void *data ATTRIBUTE_UNUSED) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("%s not implemented on Win32"), __FUNCTION__); + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("%s not implemented on Win32"), __FUNCTION__); return -1; } diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 65cbd7e..fcfbed0 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -135,8 +135,7 @@ int virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, const char **regex, int *nvars, virStorageBackendListVolRegexFunc func, - void *data, - int *exitstatus); + void *data); int virStorageBackendRunProgNul(virStoragePoolObjPtr pool, const char **prog, diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0a6b074..b8d4d63 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -233,7 +233,6 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE }; const char *prog[] = { SHOWMOUNT, "--no-headers", "--exports", NULL, NULL }; virStoragePoolSourcePtr source = NULL; - int exitstatus; char *retval = NULL; unsigned int i; @@ -246,8 +245,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE prog[3] = source->host.name; if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars, - virStorageBackendFileSystemNetFindPoolSourcesFunc, - &state, &exitstatus) < 0) + virStorageBackendFileSystemNetFindPoolSourcesFunc, + &state) < 0) goto cleanup; retval = virStoragePoolSourceListFormat(&state.list); diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index bdd58cd..15b5862 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -160,8 +160,7 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, regexes, vars, virStorageBackendISCSIExtractSession, - &session, - NULL) < 0) + &session) < 0) return NULL; if (session == NULL && @@ -503,7 +502,6 @@ virStorageBackendISCSIScanTargets(const char *portal, }; struct virStorageBackendISCSITargetList list; int i; - int exitstatus; memset(&list, 0, sizeof(list)); @@ -513,17 +511,7 @@ virStorageBackendISCSIScanTargets(const char *portal, regexes, vars, virStorageBackendISCSIGetTargets, - &list, - &exitstatus) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("iscsiadm command failed")); - return -1; - } - - if (exitstatus != 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("iscsiadm sendtargets command failed with exitstatus %d"), - exitstatus); + &list) < 0) { return -1; } diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index a1ff35b..7d5adf9 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -205,25 +205,13 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, pool->def->source.name, NULL }; - int exitstatus; - if (virStorageBackendRunProgRegex(pool, prog, 1, regexes, vars, virStorageBackendLogicalMakeVol, - vol, - &exitstatus) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("lvs command failed")); - return -1; - } - - if (exitstatus != 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("lvs command failed with exitstatus %d"), - exitstatus); + vol) < 0) { return -1; } @@ -321,7 +309,6 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, }; const char *const prog[] = { PVS, "--noheadings", "-o", "pv_name,vg_name", NULL }; const char *const scanprog[] = { VGSCAN, NULL }; - int exitstatus; char *retval = NULL; virStoragePoolSourceList sourceList; int i; @@ -331,7 +318,7 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, * that might be hanging around, so if this fails for some reason, the * worst that happens is that scanning doesn't pick everything up */ - if (virRun(scanprog, &exitstatus) < 0) { + if (virRun(scanprog, NULL) < 0) { VIR_WARN("Failure when running vgscan to refresh physical volumes"); } @@ -339,8 +326,8 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, sourceList.type = VIR_STORAGE_POOL_LOGICAL; if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars, - virStorageBackendLogicalFindPoolSourcesFunc, - &sourceList, &exitstatus) < 0) + virStorageBackendLogicalFindPoolSourcesFunc, + &sourceList) < 0) return NULL; retval = virStoragePoolSourceListFormat(&sourceList); @@ -489,7 +476,6 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, "--nosuffix", "--options", "vg_size,vg_free", pool->def->source.name, NULL }; - int exitstatus; virFileWaitForDevices(); @@ -506,13 +492,7 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, regexes, vars, virStorageBackendLogicalRefreshPoolFunc, - NULL, - &exitstatus) < 0) { - virStoragePoolObjClearVols(pool); - return -1; - } - - if (exitstatus != 0) { + NULL) < 0) { virStoragePoolObjClearVols(pool); return -1; } -- 1.7.4.4
On 05/13/2011 02:10 PM, Cole Robinson wrote:
Some clients overwrite the error from the regex helper, or do half-baked error reporting with the exitstatus.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage/storage_backend.c | 11 +++++------ src/storage/storage_backend.h | 3 +-- src/storage/storage_backend_fs.c | 5 ++--- src/storage/storage_backend_iscsi.c | 16 ++-------------- src/storage/storage_backend_logical.c | 30 +++++------------------------- 5 files changed, 15 insertions(+), 50 deletions(-)
- ret = virCommandWait(cmd, outexit); + ret = virCommandWait(cmd, NULL);
My only concern with this is that if we were previously being tolerant of a lousy child program that exits with non-zero status even when successful, we now fail. But...
@@ -246,8 +245,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE prog[3] = source->host.name;
if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars, - virStorageBackendFileSystemNetFindPoolSourcesFunc, - &state, &exitstatus) < 0) + virStorageBackendFileSystemNetFindPoolSourcesFunc, + &state) < 0)
...this caller was previously ignoring 'showmount' failures, which looks like it has sane exit status,
+++ b/src/storage/storage_backend_iscsi.c @@ -160,8 +160,7 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, regexes, vars, virStorageBackendISCSIExtractSession, - &session, - NULL) < 0) + &session) < 0)
...this caller was already rejecting failures,
@@ -513,17 +511,7 @@ virStorageBackendISCSIScanTargets(const char *portal, regexes, vars, virStorageBackendISCSIGetTargets, - &list, - &exitstatus) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("iscsiadm command failed")); - return -1; - } - - if (exitstatus != 0) {
...this caller was manually rejecting errors itself,
if (virStorageBackendRunProgRegex(pool, prog, 1, regexes, vars, virStorageBackendLogicalMakeVol, - vol, - &exitstatus) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("lvs command failed")); - return -1; - } - - if (exitstatus != 0) {
...as was this,
@@ -331,7 +318,7 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, * that might be hanging around, so if this fails for some reason, the * worst that happens is that scanning doesn't pick everything up */ - if (virRun(scanprog, &exitstatus) < 0) { + if (virRun(scanprog, NULL) < 0) { VIR_WARN("Failure when running vgscan to refresh physical volumes"); }
@@ -339,8 +326,8 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, sourceList.type = VIR_STORAGE_POOL_LOGICAL;
if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars, - virStorageBackendLogicalFindPoolSourcesFunc, - &sourceList, &exitstatus) < 0) + virStorageBackendLogicalFindPoolSourcesFunc, + &sourceList) < 0)
...another one ignoring failures, but pvs and vgscan look sane,
@@ -506,13 +492,7 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, regexes, vars, virStorageBackendLogicalRefreshPoolFunc, - NULL, - &exitstatus) < 0) { - virStoragePoolObjClearVols(pool); - return -1; - } - - if (exitstatus != 0) {
...and one last doing a manual reporting. Looks safe to me, so: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
virRunWithHook is now unused, so we can drop it. Tested w/ raw + qcow2 volume creation and copying. v2: Use opaque data to skip hook second time around Simply command building Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 - src/storage/storage_backend.c | 162 ++++++++++++++++------------------------ src/util/util.c | 23 ++---- src/util/util.h | 3 - 4 files changed, 71 insertions(+), 118 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 21a65ad..8cf9c1a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -957,7 +957,6 @@ virPipeReadUntilEOF; virRandom; virRandomInitialize; virRun; -virRunWithHook; virSetBlocking; virSetCloseExec; virSetInherit; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 61d14b1..76dca7b 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -523,8 +523,17 @@ cleanup: return ret; } +struct hookdata { + virStorageVolDefPtr vol; + bool skip; +}; + static int virStorageBuildSetUIDHook(void *data) { - virStorageVolDefPtr vol = data; + struct hookdata *tmp = data; + virStorageVolDefPtr vol = tmp->vol; + + if (tmp->skip) + return 0; if ((vol->target.perms.gid != -1) && (setgid(vol->target.perms.gid) != 0)) { @@ -545,11 +554,12 @@ static int virStorageBuildSetUIDHook(void *data) { static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, virStorageVolDefPtr vol, - const char **cmdargv) { + virCommandPtr cmd) { struct stat st; gid_t gid; uid_t uid; int filecreated = 0; + struct hookdata data = {vol, false}; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((getuid() == 0) @@ -557,21 +567,25 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, && (vol->target.perms.uid != 0)) || ((vol->target.perms.gid != -1) && (vol->target.perms.gid != getgid())))) { - if (virRunWithHook(cmdargv, - virStorageBuildSetUIDHook, vol, NULL) == 0) { + + virCommandSetPreExecHook(cmd, virStorageBuildSetUIDHook, &data); + + if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, check if the file was created */ if (stat(vol->target.path, &st) >=0) filecreated = 1; } } + + data.skip = true; + if (!filecreated) { - if (virRun(cmdargv, NULL) < 0) { + if (virCommandRun(cmd, NULL) < 0) { return -1; } if (stat(vol->target.path, &st) < 0) { virReportSystemError(errno, - _("%s failed to create %s"), - cmdargv[0], vol->target.path); + _("failed to create %s"), vol->target.path); return -1; } } @@ -644,6 +658,8 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, char *size = NULL; char *create_tool; int imgformat = -1; + virCommandPtr cmd = NULL; + bool do_encryption = (vol->target.encryption != NULL); const char *type = virStorageFileFormatTypeToString(vol->target.format); const char *backingType = vol->backingStore.path ? @@ -716,7 +732,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, } } - if (vol->target.encryption != NULL) { + if (do_encryption) { virStorageEncryptionPtr enc; if (vol->target.format != VIR_STORAGE_FILE_QCOW && @@ -767,114 +783,66 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (imgformat < 0) goto cleanup; + cmd = virCommandNew(create_tool); + if (inputvol) { - const char *imgargv[] = { - create_tool, - "convert", - "-f", inputType, - "-O", type, - inputPath, - vol->target.path, - NULL, - NULL, - NULL - }; - - if (vol->target.encryption != NULL) { + virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, + inputPath, vol->target.path, NULL); + + if (do_encryption) { if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { - imgargv[8] = "-o"; - imgargv[9] = "encryption=on"; + virCommandAddArgList(cmd, "-o", "encryption=on", NULL); } else { - imgargv[8] = "-e"; + virCommandAddArg(cmd, "-e"); } } - ret = virStorageBackendCreateExecCommand(pool, vol, imgargv); } else if (vol->backingStore.path) { - const char *imgargv[] = { - create_tool, - "create", - "-f", type, - "-b", vol->backingStore.path, - NULL, - NULL, - NULL, - NULL, - NULL, - NULL - }; - - char *optflag = NULL; + virCommandAddArgList(cmd, "create", "-f", type, + "-b", vol->backingStore.path, NULL); + switch (imgformat) { case QEMU_IMG_BACKING_FORMAT_FLAG: - imgargv[6] = "-F"; - imgargv[7] = backingType; - imgargv[8] = vol->target.path; - imgargv[9] = size; - if (vol->target.encryption != NULL) - imgargv[10] = "-e"; + virCommandAddArgList(cmd, "-F", backingType, vol->target.path, + size, NULL); + + if (do_encryption) + virCommandAddArg(cmd, "-e"); break; case QEMU_IMG_BACKING_FORMAT_OPTIONS: - if (virAsprintf(&optflag, "backing_fmt=%s", backingType) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (vol->target.encryption != NULL) { - char *tmp = NULL; - if (virAsprintf(&tmp, "%s,%s", optflag, "encryption=on") < 0) { - virReportOOMError(); - goto cleanup; - } - VIR_FREE(optflag); - optflag = tmp; - } - - imgargv[6] = "-o"; - imgargv[7] = optflag; - imgargv[8] = vol->target.path; - imgargv[9] = size; + virCommandAddArg(cmd, "-o"); + virCommandAddArgFormat(cmd, "backing_fmt=%s%s", backingType, + do_encryption ? ",encryption=on" : ""); + virCommandAddArgList(cmd, vol->target.path, size, NULL); + break; default: VIR_INFO("Unable to set backing store format for %s with %s", vol->target.path, create_tool); - imgargv[6] = vol->target.path; - imgargv[7] = size; - if (vol->target.encryption != NULL) - imgargv[8] = "-e"; - } - ret = virStorageBackendCreateExecCommand(pool, vol, imgargv); - VIR_FREE(optflag); + virCommandAddArgList(cmd, vol->target.path, size, NULL); + if (do_encryption) + virCommandAddArg(cmd, "-e"); + } } else { - /* The extra NULL field is for indicating encryption (-e). */ - const char *imgargv[] = { - create_tool, - "create", - "-f", type, - vol->target.path, - size, - NULL, - NULL, - NULL - }; - - if (vol->target.encryption != NULL) { + virCommandAddArgList(cmd, "create", "-f", type, + vol->target.path, size, NULL); + + if (do_encryption) { if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { - imgargv[6] = "-o"; - imgargv[7] = "encryption=on"; + virCommandAddArgList(cmd, "-o", "encryption=on", NULL); } else { - imgargv[6] = "-e"; + virCommandAddArg(cmd, "-e"); } } - - ret = virStorageBackendCreateExecCommand(pool, vol, imgargv); } - cleanup: + ret = virStorageBackendCreateExecCommand(pool, vol, cmd); +cleanup: VIR_FREE(size); VIR_FREE(create_tool); + virCommandFree(cmd); return ret; } @@ -892,7 +860,8 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, { int ret; char *size; - const char *imgargv[4]; + char *create_tool; + virCommandPtr cmd; if (inputvol) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -926,13 +895,12 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } - imgargv[0] = virFindFileInPath("qcow-create"); - imgargv[1] = size; - imgargv[2] = vol->target.path; - imgargv[3] = NULL; + create_tool = virFindFileInPath("qcow-create"); + cmd = virCommandNewArgList(create_tool, size, vol->target.path, NULL); - ret = virStorageBackendCreateExecCommand(pool, vol, imgargv); - VIR_FREE(imgargv[0]); + ret = virStorageBackendCreateExecCommand(pool, vol, cmd); + virCommandFree(cmd); + VIR_FREE(create_tool); VIR_FREE(size); return ret; diff --git a/src/util/util.c b/src/util/util.c index f169e54..89fc82b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -787,10 +787,8 @@ virExec(const char *const*argv, * only if the command could not be run. */ int -virRunWithHook(const char *const*argv, - virExecHook hook, - void *data, - int *status) { +virRun(const char *const*argv, int *status) +{ pid_t childpid; int exitstatus, execret, waitret; int ret = -1; @@ -807,7 +805,7 @@ virRunWithHook(const char *const*argv, if ((execret = virExecWithHook(argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, hook, data, NULL)) < 0) { + VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { ret = execret; goto error; } @@ -871,17 +869,14 @@ int virSetInherit(int fd ATTRIBUTE_UNUSED, bool inherit ATTRIBUTE_UNUSED) return -1; } -int -virRunWithHook(const char *const *argv ATTRIBUTE_UNUSED, - virExecHook hook ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED, - int *status) +virRun(const char *const *argv ATTRIBUTE_UNUSED, + int *status) { if (status) *status = ENOTSUP; else virUtilError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virRunWithHook is not implemented for WIN32")); + "%s", _("virRun is not implemented for WIN32")); return -1; } @@ -1018,12 +1013,6 @@ error: return -1; } -int -virRun(const char *const*argv, - int *status) { - return virRunWithHook(argv, NULL, NULL, status); -} - /* Like gnulib's fread_file, but read no more than the specified maximum number of bytes. If the length of the input is <= max_len, and upon error while reading that data, it works just like fread_file. */ diff --git a/src/util/util.h b/src/util/util.h index 482294f..e2b8eb3 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -78,9 +78,6 @@ int virExec(const char *const*argv, int *errfd, int flags) ATTRIBUTE_RETURN_CHECK; int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; -int virRunWithHook(const char *const*argv, - virExecHook hook, void *data, - int *status) ATTRIBUTE_RETURN_CHECK; int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf); int virFork(pid_t *pid); -- 1.7.4.4
On 05/13/2011 02:10 PM, Cole Robinson wrote:
virRunWithHook is now unused, so we can drop it. Tested w/ raw + qcow2 volume creation and copying.
v2: Use opaque data to skip hook second time around Simply command building
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 - src/storage/storage_backend.c | 162 ++++++++++++++++------------------------ src/util/util.c | 23 ++---- src/util/util.h | 3 - 4 files changed, 71 insertions(+), 118 deletions(-)
@@ -926,13 +895,12 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; }
- imgargv[0] = virFindFileInPath("qcow-create"); - imgargv[1] = size; - imgargv[2] = vol->target.path; - imgargv[3] = NULL; + create_tool = virFindFileInPath("qcow-create"); + cmd = virCommandNewArgList(create_tool, size, vol->target.path, NULL);
Nit - virCommand (via virExecWithHook) already calls virFindFileInPath(argv[0]) if you pass a relative file name, so you could simplify this to: virCommandNewArgList("qcow-create", size, ...) ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/13/2011 06:13 PM, Eric Blake wrote:
On 05/13/2011 02:10 PM, Cole Robinson wrote:
virRunWithHook is now unused, so we can drop it. Tested w/ raw + qcow2 volume creation and copying.
v2: Use opaque data to skip hook second time around Simply command building
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 - src/storage/storage_backend.c | 162 ++++++++++++++++------------------------ src/util/util.c | 23 ++---- src/util/util.h | 3 - 4 files changed, 71 insertions(+), 118 deletions(-)
@@ -926,13 +895,12 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; }
- imgargv[0] = virFindFileInPath("qcow-create"); - imgargv[1] = size; - imgargv[2] = vol->target.path; - imgargv[3] = NULL; + create_tool = virFindFileInPath("qcow-create"); + cmd = virCommandNewArgList(create_tool, size, vol->target.path, NULL);
Nit - virCommand (via virExecWithHook) already calls virFindFileInPath(argv[0]) if you pass a relative file name, so you could simplify this to:
virCommandNewArgList("qcow-create", size, ...)
ACK.
Thanks for the review, pushed with that fix. Also pushed 1, 4, and 5. - Cole
On Fri, May 13, 2011 at 04:10:34PM -0400, Cole Robinson wrote:
virRunWithHook is now unused, so we can drop it. Tested w/ raw + qcow2 volume creation and copying.
v2: Use opaque data to skip hook second time around Simply command building
Signed-off-by: Cole Robinson <crobinso@redhat.com> @@ -871,17 +869,14 @@ int virSetInherit(int fd ATTRIBUTE_UNUSED, bool inherit ATTRIBUTE_UNUSED) return -1; }
-int -virRunWithHook(const char *const *argv ATTRIBUTE_UNUSED, - virExecHook hook ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED, - int *status) +virRun(const char *const *argv ATTRIBUTE_UNUSED, + int *status)
This broke Win32 builds, as it lost the return type for 'virRun'. I pushed this fix: commit 00e74007cb959399a1cfaefb967827e59d961609 Author: Daniel P. Berrange <berrange@redhat.com> Date: Tue May 17 16:22:49 2011 +0100 Fix prototype of virRun for Win32 targets * src/util/util.c: Fix virRun prototype diff --git a/src/util/util.c b/src/util/util.c index f07b8b0..d1a08a6 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -869,6 +869,7 @@ int virSetInherit(int fd ATTRIBUTE_UNUSED, bool inherit ATTRIBUTE_UNUSED) return -1; } +int virRun(const char *const *argv ATTRIBUTE_UNUSED, int *status) { Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 - src/util/util.c | 35 ----------------------------------- src/util/util.h | 8 -------- 3 files changed, 0 insertions(+), 44 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8cf9c1a..6066e16 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -914,7 +914,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/13/2011 02:10 PM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 - src/util/util.c | 35 ----------------------------------- src/util/util.h | 8 -------- 3 files changed, 0 insertions(+), 44 deletions(-)
Oops - division by zero when I tried to continue my trend of pointing out the nice patch ratio in this series :) And in spite of that, I still found a problem with it - you didn't delete enough :-) ACK with this squashed in: diff --git i/cfg.mk w/cfg.mk index 06b638b..eba4462 100644 --- i/cfg.mk +++ w/cfg.mk @@ -261,7 +261,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' \ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
v2: Simplify command building Handle command building failure Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/util.c | 74 ++++-------------------------------------------------- 1 files changed, 6 insertions(+), 68 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 7679834..2fe3808 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -769,76 +769,14 @@ 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; - } + int ret; + virCommandPtr cmd = virCommandNewArgs(argv); - ret = 0; + if (!cmd) + return -1; - 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/13/2011 02:10 PM, Cole Robinson wrote:
v2: Simplify command building Handle command building failure
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/util.c | 74 ++++-------------------------------------------------- 1 files changed, 6 insertions(+), 68 deletions(-)
fun 1:11 ratio
+ int ret; + virCommandPtr cmd = virCommandNewArgs(argv);
- ret = 0; + if (!cmd) + return -1;
You should not add these two lines. Why? Because you didn't report the OOM error,
- error: - VIR_FREE(outbuf); - VIR_FREE(errbuf); - VIR_FORCE_CLOSE(outfd); - VIR_FORCE_CLOSE(errfd); + ret = virCommandRun(cmd, status);
but this safely handles NULL cmd on input and correctly reports the OOM error. ACK with that buglet fixed. -- 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 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 | 568 +++++++++++++++++++++++++++++ src/util/command.h | 14 + src/util/ebtables.c | 2 +- src/util/pci.c | 2 +- src/util/util.c | 568 +---------------------------- src/util/util.h | 24 -- src/vmware/vmware_driver.c | 1 + 13 files changed, 602 insertions(+), 591 deletions(-) diff --git a/cfg.mk b/cfg.mk index 06b638b..db11d6e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -621,7 +621,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 6066e16..a3c8877 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -130,6 +130,8 @@ virCommandTransferFD; virCommandTranslateStatus; virCommandWait; virCommandWriteArgLog; +virFork; +virRun; # conf.h @@ -914,7 +916,6 @@ virEnumFromString; virEnumToString; virEventAddHandle; virEventRemoveHandle; -virExecWithHook; virFileAbsPath; virFileDeletePid; virFileExists; @@ -936,7 +937,6 @@ virFileStripSuffix; virFileWaitForDevices; virFileWriteStr; virFindFileInPath; -virFork; virFormatMacAddr; virGenerateMacAddr; virGetGroupID; @@ -955,7 +955,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 b488d55..3c1b3f2 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -26,6 +26,12 @@ #include <stdlib.h> #include <sys/stat.h> #include <sys/wait.h> +#include <sys/types.h> +#include <fcntl.h> + +#if HAVE_CAPNG +# include <cap-ng.h> +#endif #include "command.h" #include "memory.h" @@ -34,6 +40,7 @@ #include "logging.h" #include "files.h" #include "buf.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -41,6 +48,14 @@ virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) +/* Flags for virExecWithHook */ +enum { + VIR_EXEC_NONE = 0, + VIR_EXEC_NONBLOCK = (1 << 0), + VIR_EXEC_DAEMON = (1 << 1), + VIR_EXEC_CLEAR_CAPS = (1 << 2), +}; + enum { /* Internal-use extension beyond public VIR_EXEC_ flags */ VIR_EXEC_RUN_SYNC = 0x40000000, @@ -84,6 +99,559 @@ 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_WARN0("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 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 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); + + if (!cmd) + return -1; + + 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 2fe3808..70b54c7 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -34,11 +34,11 @@ #include <errno.h> #include <poll.h> #include <time.h> -#include <sys/types.h> -#include <sys/stat.h> #include <sys/ioctl.h> #include <sys/wait.h> #include <sys/time.h> +#include <sys/stat.h> +#include <sys/types.h> #if HAVE_MMAP # include <sys/mman.h> #endif @@ -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,510 +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); - - if (!cmd) - return -1; - - ret = virCommandRun(cmd, status); - virCommandFree(cmd); - return ret; -} - #else /* WIN32 */ int virSetInherit(int fd ATTRIBUTE_UNUSED, bool inherit ATTRIBUTE_UNUSED) @@ -787,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 b920f79..34f8ff6 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/13/2011 02:10 PM, Cole Robinson wrote:
Seems reasonable to have all command wrappers in the same place
v2: Dont move SetInherit
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 | 568 +++++++++++++++++++++++++++++ src/util/command.h | 14 + src/util/ebtables.c | 2 +- src/util/pci.c | 2 +- src/util/util.c | 568 +---------------------------- src/util/util.h | 24 -- src/vmware/vmware_driver.c | 1 + 13 files changed, 602 insertions(+), 591 deletions(-)
@@ -914,7 +916,6 @@ virEnumFromString; virEnumToString; virEventAddHandle; virEventRemoveHandle; -virExecWithHook;
Did we miss this hunk in an earlier patch? Oh well, at least we're fixing it here.
+++ b/src/util/command.c @@ -26,6 +26,12 @@ #include <stdlib.h> #include <sys/stat.h> #include <sys/wait.h> +#include <sys/types.h>
You can probably drop the <sys/types.h> line. POSIX (and gnulib) guarantee that most other headers are self-contained.
+/* Flags for virExecWithHook */ +enum { + VIR_EXEC_NONE = 0, + VIR_EXEC_NONBLOCK = (1 << 0), + VIR_EXEC_DAEMON = (1 << 1), + VIR_EXEC_CLEAR_CAPS = (1 << 2), +}; + enum { /* Internal-use extension beyond public VIR_EXEC_ flags */ VIR_EXEC_RUN_SYNC = 0x40000000,
Oops, you missed my review comment from v1 that these two enums should be merged into one, now that _all_ flags are internal use only.
+# else +static int virClearCapabilities(void) +{ +// VIR_WARN0("libcap-ng support not compiled in, unable to clear capabilities");
s/VIR_WARN0/VIR_WARN/ so that if we ever uncomment this, it will compile.
+ if (forkRet < 0) { + /* The fork was sucessful, but after that there was an error
s/sucessful/successful/
+++ b/src/util/util.c @@ -34,11 +34,11 @@ #include <errno.h> #include <poll.h> #include <time.h> -#include <sys/types.h> -#include <sys/stat.h> #include <sys/ioctl.h> #include <sys/wait.h> #include <sys/time.h> +#include <sys/stat.h> +#include <sys/types.h>
Looks like a spurious hunk. Were you trying to sort lines? You can probably drop <sys/types.h>. ACK with those nits addressed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake