util: make it easier to grab only regular command exit

Auditing all callers of virCommandRun and virCommandWait that
passed a non-NULL pointer for exit status turned up some
interesting observations.  Many callers were merely passing
a pointer to avoid the overall command dying, but without
caring what the exit status was - but these callers would
be better off treating a child death by signal as an abnormal
exit.  Other callers were actually acting on the status, but
not all of them remembered to filter by WIFEXITED and convert
with WEXITSTATUS; depending on the platform, this can result
in a status being reported as 256 times too big.  And among
those that correctly parse the output, it gets rather verbose.
Finally, there were the callers that explicitly checked that
the status was 0, and gave their own message, but with fewer
details than what virCommand gives for free.

So the best idea is to move the complexity out of callers and
into virCommand - by default, we return the actual exit status
already cleaned through WEXITSTATUS and treat signals as a
failed command; but the few callers that care can ask for raw
status and act on it themselves.

* src/util/vircommand.h (virCommandRawStatus): New prototype.
* src/libvirt_private.syms (util/command.h): Export it.
* docs/internals/command.html.in: Document it.
* src/util/vircommand.c (virCommandRawStatus): New function.
(virCommandWait): Adjust semantics.
* tests/commandtest.c (test1): Test it.
* daemon/remote.c (remoteDispatchAuthPolkit): Adjust callers.
* src/access/viraccessdriverpolkit.c (virAccessDriverPolkitCheck):
Likewise.
* src/fdstream.c (virFDStreamCloseInt): Likewise.
* src/lxc/lxc_process.c (virLXCProcessStart): Likewise.
* src/qemu/qemu_command.c (qemuCreateInBridgePortWithHelper):
Likewise.
* src/xen/xen_driver.c (xenUnifiedXendProbe): Simplify.
* tests/reconnect.c (mymain): Likewise.
* tests/statstest.c (mymain): Likewise.
* src/bhyve/bhyve_process.c (virBhyveProcessStart)
(virBhyveProcessStop): Don't overwrite virCommand error.
* src/libvirt.c (virConnectAuthGainPolkit): Likewise.
* src/openvz/openvz_driver.c (openvzDomainGetBarrierLimit)
(openvzDomainSetBarrierLimit): Likewise.
* src/util/virebtables.c (virEbTablesOnceInit): Likewise.
* src/util/viriptables.c (virIpTablesOnceInit): Likewise.
* src/util/virnetdevveth.c (virNetDevVethCreate): Fix debug
message.
* src/qemu/qemu_capabilities.c (virQEMUCapsInitQMP): Add comment.
* src/storage/storage_backend_iscsi.c
(virStorageBackendISCSINodeUpdate): Likewise.

Signed-off-by: Eric Blake <eblake@redhat.com>
This commit is contained in:
Eric Blake 2014-02-19 17:32:19 -07:00
parent c72e76c3d9
commit b9dd878ff8
21 changed files with 117 additions and 77 deletions

View File

@ -3129,10 +3129,9 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED,
authdismissed = (pkout && strstr(pkout, "dismissed=true"));
if (status != 0) {
char *tmp = virProcessTranslateStatus(status);
VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d: %s"),
action, (long long) callerPid, callerUid, NULLSTR(tmp));
VIR_FREE(tmp);
VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d "
"with status %d"),
action, (long long) callerPid, callerUid, status);
goto authdeny;
}
PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW,

View File

@ -430,7 +430,7 @@
if (string)
VIR_DEBUG("about to run %s", string);
VIR_FREE(string);
if (virCommandRun(cmd) &lt; 0)
if (virCommandRun(cmd, NULL) &lt; 0)
return -1;
</pre>
@ -458,15 +458,24 @@
non-zero exit status can represent a success condition,
it is possible to request the exit status and perform
that check manually instead of letting <code>virCommandRun</code>
raise the error
raise the error. By default, the captured status is only
for a normal exit (death from a signal is treated as an error),
but a caller can use <code>virCommandRawStatus</code> to get
encoded status that includes any terminating signals.
</p>
<pre>
int status;
if (virCommandRun(cmd, &amp;status) &lt; 0)
return -1;
return -1;
if (status == 1) {
...do stuff...
}
if (WEXITSTATUS(status) ...) {
virCommandRawStatus(cmd2);
if (virCommandRun(cmd2, &amp;status) &lt; 0)
return -1;
if (WIFEXITED(status) &amp;&amp; WEXITSTATUS(status) == 1) {
...do stuff...
}
</pre>

View File

@ -1,7 +1,7 @@
/*
* viraccessdriverpolkit.c: polkited access control driver
*
* Copyright (C) 2012 Red Hat, Inc.
* Copyright (C) 2012, 2014 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@ -170,11 +170,10 @@ virAccessDriverPolkitCheck(virAccessManagerPtr manager ATTRIBUTE_UNUSED,
ret = 0; /* Denied */
} else {
ret = -1; /* Error */
char *tmp = virProcessTranslateStatus(status);
virAccessError(VIR_ERR_ACCESS_DENIED,
_("Policy kit denied action %s from %s: %s"),
actionid, process, NULLSTR(tmp));
VIR_FREE(tmp);
_("Policy kit denied action %s from %s: "
"exit status %d"),
actionid, process, status);
}
goto cleanup;
}

View File

@ -57,7 +57,7 @@ virBhyveProcessStart(virConnectPtr conn,
virCommandPtr cmd = NULL;
virCommandPtr load_cmd = NULL;
bhyveConnPtr privconn = conn->privateData;
int ret = -1, status;
int ret = -1;
if (virAsprintf(&logfile, "%s/%s.log",
BHYVE_LOG_DIR, vm->def->name) < 0)
@ -114,15 +114,9 @@ virBhyveProcessStart(virConnectPtr conn,
virStrerror(errno, ebuf, sizeof(ebuf)));
VIR_DEBUG("Loading domain '%s'", vm->def->name);
if (virCommandRun(load_cmd, &status) < 0)
if (virCommandRun(load_cmd, NULL) < 0)
goto cleanup;
if (status != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Guest failed to load: %d"), status);
goto cleanup;
}
/* Now we can start the domain */
VIR_DEBUG("Starting domain '%s'", vm->def->name);
ret = virCommandRun(cmd, NULL);
@ -165,7 +159,6 @@ virBhyveProcessStop(bhyveConnPtr driver,
{
size_t i;
int ret = -1;
int status;
virCommandPtr cmd = NULL;
if (!virDomainObjIsActive(vm)) {
@ -203,15 +196,9 @@ virBhyveProcessStop(bhyveConnPtr driver,
if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm)))
goto cleanup;
if (virCommandRun(cmd, &status) < 0)
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
if (status != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Guest failed to stop: %d"), status);
goto cleanup;
}
ret = 0;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);

View File

@ -1,7 +1,7 @@
/*
* fdstream.c: generic streams impl for file descriptors
*
* Copyright (C) 2009-2012 Red Hat, Inc.
* Copyright (C) 2009-2012, 2014 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@ -302,6 +302,7 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)
else
buf[len] = '\0';
virCommandRawStatus(fdst->cmd);
if (virCommandWait(fdst->cmd, &status) < 0) {
ret = -1;
} else if (status != 0) {

View File

@ -145,15 +145,13 @@ static int
virConnectAuthGainPolkit(const char *privilege)
{
virCommandPtr cmd;
int status;
int ret = -1;
if (geteuid() == 0)
return 0;
cmd = virCommandNewArgList(POLKIT_AUTH, "--obtain", privilege, NULL);
if (virCommandRun(cmd, &status) < 0 ||
status > 0)
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
ret = 0;

View File

@ -1108,6 +1108,7 @@ virCommandNewArgList;
virCommandNewArgs;
virCommandNonblockingFDs;
virCommandPassFD;
virCommandRawStatus;
virCommandRequireHandshake;
virCommandRun;
virCommandRunAsync;

View File

@ -1224,12 +1224,19 @@ int virLXCProcessStart(virConnectPtr conn,
VIR_WARN("Unable to seek to end of logfile: %s",
virStrerror(errno, ebuf, sizeof(ebuf)));
virCommandRawStatus(cmd);
if (virCommandRun(cmd, &status) < 0)
goto cleanup;
if (status != 0) {
if (virLXCProcessReadLogOutput(vm, logfile, pos, ebuf, sizeof(ebuf)) <= 0)
snprintf(ebuf, sizeof(ebuf), "unexpected exit status %d", status);
if (virLXCProcessReadLogOutput(vm, logfile, pos, ebuf,
sizeof(ebuf)) <= 0) {
if (WIFEXITED(status))
snprintf(ebuf, sizeof(ebuf), _("unexpected exit status %d"),
WEXITSTATUS(status));
else
snprintf(ebuf, sizeof(ebuf), "%s", _("terminated abnormally"));
}
virReportError(VIR_ERR_INTERNAL_ERROR,
_("guest failed to start: %s"), ebuf);
goto cleanup;

View File

@ -1,7 +1,7 @@
/*
* openvz_driver.c: core driver methods for managing OpenVZ VEs
*
* Copyright (C) 2010-2013 Red Hat, Inc.
* Copyright (C) 2010-2014 Red Hat, Inc.
* Copyright (C) 2006, 2007 Binary Karma
* Copyright (C) 2006 Shuveb Hussain
* Copyright (C) 2007 Anoop Joe Cyriac
@ -1710,7 +1710,7 @@ openvzDomainGetBarrierLimit(virDomainPtr domain,
unsigned long long *barrier,
unsigned long long *limit)
{
int status, ret = -1;
int ret = -1;
char *endp, *output = NULL;
const char *tmp;
virCommandPtr cmd = virCommandNewArgList(VZLIST, "--no-header", NULL);
@ -1718,12 +1718,8 @@ openvzDomainGetBarrierLimit(virDomainPtr domain,
virCommandSetOutputBuffer(cmd, &output);
virCommandAddArgFormat(cmd, "-o%s.b,%s.l", param, param);
virCommandAddArg(cmd, domain->name);
if (virCommandRun(cmd, &status) < 0 || status != 0) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("Failed to get %s for %s: %d"), param, domain->name,
status);
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
}
tmp = output;
virSkipSpaces(&tmp);
@ -1754,7 +1750,7 @@ openvzDomainSetBarrierLimit(virDomainPtr domain,
unsigned long long barrier,
unsigned long long limit)
{
int status, ret = -1;
int ret = -1;
virCommandPtr cmd = virCommandNewArgList(VZCTL, "--quiet", "set", NULL);
/* LONG_MAX indicates unlimited so reject larger values */
@ -1769,12 +1765,8 @@ openvzDomainSetBarrierLimit(virDomainPtr domain,
virCommandAddArgFormat(cmd, "--%s", param);
virCommandAddArgFormat(cmd, "%llu:%llu", barrier, limit);
virCommandAddArg(cmd, "--save");
if (virCommandRun(cmd, &status) < 0 || status != 0) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("Failed to set %s for %s: %d"), param, domain->name,
status);
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
}
ret = 0;
cleanup:

View File

@ -2693,6 +2693,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
virCommandSetGID(cmd, runGid);
virCommandSetUID(cmd, runUid);
/* Log, but otherwise ignore, non-zero status. */
if (virCommandRun(cmd, &status) < 0)
goto cleanup;

View File

@ -232,7 +232,6 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
unsigned int flags)
{
virCommandPtr cmd;
int status;
int pair[2] = { -1, -1 };
if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP)
@ -269,7 +268,7 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
}
if (virNetDevTapGetName(*tapfd, ifname) < 0 ||
virCommandWait(cmd, &status) < 0) {
virCommandWait(cmd, NULL) < 0) {
VIR_FORCE_CLOSE(*tapfd);
*tapfd = -1;
}

View File

@ -1,7 +1,7 @@
/*
* storage_backend_iscsi.c: storage backend for iSCSI handling
*
* Copyright (C) 2007-2008, 2010-2012 Red Hat, Inc.
* Copyright (C) 2007-2014 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@ -111,10 +111,6 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool,
virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL);
/* Note that we ignore the exitstatus. Older versions of iscsiadm tools
* returned an exit status of > 0, even if they succeeded. We will just
* rely on whether session got filled in properly.
*/
if (virStorageBackendRunProgRegex(pool,
cmd,
1,
@ -681,6 +677,7 @@ virStorageBackendISCSINodeUpdate(const char *portal,
"--value", value,
NULL);
/* Ignore non-zero status. */
if (virCommandRun(cmd, &status) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to update '%s' of node mode for target '%s'"),

View File

@ -113,6 +113,7 @@ struct _virCommand {
pid_t pid;
char *pidfile;
bool reap;
bool rawStatus;
unsigned long long maxMemLock;
unsigned int maxProcesses;
@ -1120,6 +1121,25 @@ virCommandNonblockingFDs(virCommandPtr cmd)
cmd->flags |= VIR_EXEC_NONBLOCK;
}
/**
* virCommandRawStatus:
* @cmd: the command to modify
*
* Mark this command as returning raw exit status via virCommandRun() or
* virCommandWait() (caller must use WIFEXITED() and friends, and can
* detect death from signals) instead of the default of only allowing
* normal exit status (caller must not use WEXITSTATUS(), and death from
* signals returns -1).
*/
void
virCommandRawStatus(virCommandPtr cmd)
{
if (!cmd || cmd->has_error)
return;
cmd->rawStatus = true;
}
/* Add an environment variable to the cmd->env list. 'env' is a
* string like "name=value". If the named environment variable is
* already set, then it is replaced in the list.
@ -2045,7 +2065,11 @@ int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED)
* Returns -1 on any error executing the
* command. Returns 0 if the command executed,
* with the exit status set. If @exitstatus is NULL, then the
* child must exit with status 0 for this to succeed.
* child must exit with status 0 for this to succeed. By default,
* a non-NULL @exitstatus contains the normal exit status of the child
* (death from a signal is treated as execution error); but if
* virCommandRawStatus() was used, it instead contains the raw exit
* status that the caller must then decipher using WIFEXITED() and friends.
*/
int
virCommandRun(virCommandPtr cmd, int *exitstatus)
@ -2335,7 +2359,11 @@ cleanup:
* to complete. Return -1 on any error waiting for
* completion. Returns 0 if the command
* finished with the exit status set. If @exitstatus is NULL, then the
* child must exit with status 0 for this to succeed.
* child must exit with status 0 for this to succeed. By default,
* a non-NULL @exitstatus contains the normal exit status of the child
* (death from a signal is treated as execution error); but if
* virCommandRawStatus() was used, it instead contains the raw exit
* status that the caller must then decipher using WIFEXITED() and friends.
*/
int
virCommandWait(virCommandPtr cmd, int *exitstatus)
@ -2372,7 +2400,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
* message is not as detailed as what we can provide. So, we
* guarantee that virProcessWait only fails due to failure to wait,
* and repeat the exitstatus check code ourselves. */
ret = virProcessWait(cmd->pid, exitstatus ? exitstatus : &status, true);
ret = virProcessWait(cmd->pid, &status, true);
if (cmd->flags & VIR_EXEC_ASYNC_IO) {
cmd->flags &= ~VIR_EXEC_ASYNC_IO;
virThreadJoin(cmd->asyncioThread);
@ -2390,7 +2418,9 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
if (ret == 0) {
cmd->pid = -1;
cmd->reap = false;
if (status) {
if (exitstatus && (cmd->rawStatus || WIFEXITED(status))) {
*exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status);
} else if (status) {
char *str = virCommandToString(cmd);
char *st = virProcessTranslateStatus(status);
bool haveErrMsg = cmd->errbuf && *cmd->errbuf && (*cmd->errbuf)[0];

View File

@ -86,6 +86,8 @@ void virCommandDaemonize(virCommandPtr cmd);
void virCommandNonblockingFDs(virCommandPtr cmd);
void virCommandRawStatus(virCommandPtr cmd);
void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...)
ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3);

View File

@ -1,7 +1,7 @@
/*
* virebtables.c: Helper APIs for managing ebtables
*
* Copyright (C) 2007-2013 Red Hat, Inc.
* Copyright (C) 2007-2014 Red Hat, Inc.
* Copyright (C) 2009 IBM Corp.
*
* This library is free software; you can redistribute it and/or
@ -66,10 +66,9 @@ virEbTablesOnceInit(void)
"firewalld support disabled for ebtables.");
} else {
virCommandPtr cmd = virCommandNew(firewall_cmd_path);
int status;
virCommandAddArgList(cmd, "--state", NULL);
if (virCommandRun(cmd, &status) < 0 || status != 0) {
if (virCommandRun(cmd, NULL) < 0) {
VIR_INFO("firewall-cmd found but disabled for ebtables");
VIR_FREE(firewall_cmd_path);
firewall_cmd_path = NULL;

View File

@ -1,7 +1,7 @@
/*
* viriptables.c: helper APIs for managing iptables
*
* Copyright (C) 2007-2013 Red Hat, Inc.
* Copyright (C) 2007-2014 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@ -60,7 +60,6 @@ static int
virIpTablesOnceInit(void)
{
virCommandPtr cmd;
int status;
#if HAVE_FIREWALLD
firewall_cmd_path = virFindFileInPath("firewall-cmd");
@ -71,7 +70,7 @@ virIpTablesOnceInit(void)
cmd = virCommandNew(firewall_cmd_path);
virCommandAddArgList(cmd, "--state", NULL);
if (virCommandRun(cmd, &status) < 0 || status != 0) {
if (virCommandRun(cmd, NULL) < 0) {
VIR_INFO("firewall-cmd found but disabled for iptables");
VIR_FREE(firewall_cmd_path);
firewall_cmd_path = NULL;
@ -88,7 +87,7 @@ virIpTablesOnceInit(void)
cmd = virCommandNew(IPTABLES_PATH);
virCommandAddArgList(cmd, "-w", "-L", "-n", NULL);
if (virCommandRun(cmd, &status) < 0 || status != 0) {
if (virCommandRun(cmd, NULL) < 0) {
VIR_INFO("xtables locking not supported by your iptables");
} else {
VIR_INFO("using xtables locking for iptables");

View File

@ -1,5 +1,5 @@
/*
* Copyright (C) 2010-2013 Red Hat, Inc.
* Copyright (C) 2010-2014 Red Hat, Inc.
* Copyright IBM Corp. 2008
*
* This library is free software; you can redistribute it and/or
@ -183,7 +183,7 @@ int virNetDevVethCreate(char** veth1, char** veth2)
VIR_DEBUG("Failed to create veth host: %s guest: %s: %d",
*veth1 ? *veth1 : veth1auto,
*veth1 ? *veth1 : veth1auto,
*veth2 ? *veth2 : veth2auto,
status);
VIR_FREE(veth1auto);
VIR_FREE(veth2auto);

View File

@ -308,16 +308,15 @@ xenUnifiedProbe(void)
}
#ifdef WITH_LIBXL
static int
static bool
xenUnifiedXendProbe(void)
{
virCommandPtr cmd;
int status;
int ret = 0;
bool ret = false;
cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL);
if (virCommandRun(cmd, &status) == 0 && status == 0)
ret = 1;
if (virCommandRun(cmd, NULL) == 0)
ret = true;
virCommandFree(cmd);
return ret;

View File

@ -139,6 +139,12 @@ static int test1(const void *unused ATTRIBUTE_UNUSED)
int status;
cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist");
if (virCommandRun(cmd, &status) < 0)
goto cleanup;
if (status != EXIT_ENOENT)
goto cleanup;
virCommandRawStatus(cmd);
if (virCommandRun(cmd, &status) < 0)
goto cleanup;
if (!WIFEXITED(status) || WEXITSTATUS(status) != EXIT_ENOENT)
@ -909,6 +915,17 @@ test22(const void *unused ATTRIBUTE_UNUSED)
cmd = virCommandNewArgList("/bin/sh", "-c", "exit 3", NULL);
if (virCommandRun(cmd, &status) < 0) {
virErrorPtr err = virGetLastError();
printf("Cannot run child %s\n", err->message);
goto cleanup;
}
if (status != 3) {
printf("Unexpected status %d\n", status);
goto cleanup;
}
virCommandRawStatus(cmd);
if (virCommandRun(cmd, &status) < 0) {
virErrorPtr err = virGetLastError();
printf("Cannot run child %s\n", err->message);
@ -922,6 +939,12 @@ test22(const void *unused ATTRIBUTE_UNUSED)
virCommandFree(cmd);
cmd = virCommandNewArgList("/bin/sh", "-c", "kill -9 $$", NULL);
if (virCommandRun(cmd, &status) == 0) {
printf("Death by signal not detected, status %d\n", status);
goto cleanup;
}
virCommandRawStatus(cmd);
if (virCommandRun(cmd, &status) < 0) {
virErrorPtr err = virGetLastError();
printf("Cannot run child %s\n", err->message);

View File

@ -15,7 +15,6 @@ mymain(void)
bool ro = false;
virConnectPtr conn;
virDomainPtr dom;
int status;
virCommandPtr cmd;
struct utsname ut;
@ -26,7 +25,7 @@ mymain(void)
if (strstr(ut.release, "xen") == NULL)
return EXIT_AM_SKIP;
cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL);
if (virCommandRun(cmd, &status) != 0 || status != 0) {
if (virCommandRun(cmd, NULL) < 0) {
virCommandFree(cmd);
return EXIT_AM_SKIP;
}

View File

@ -40,7 +40,6 @@ static int
mymain(void)
{
int ret = 0;
int status;
virCommandPtr cmd;
struct utsname ut;
@ -51,7 +50,7 @@ mymain(void)
if (strstr(ut.release, "xen") == NULL)
return EXIT_AM_SKIP;
cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL);
if (virCommandRun(cmd, &status) != 0 || status != 0) {
if (virCommandRun(cmd, NULL) < 0) {
virCommandFree(cmd);
return EXIT_AM_SKIP;
}