[libvirt] [PATCH 0/3] Fix error reporting of virsh qemu-agent-command and underlying API's.
The code was horribly broken in multiple ways. This series fixes dispatching of errors from the API, null dereference in virsh and shadowed error messages. Peter Krempa (3): virsh-domain: Report errors and don't deref NULL in qemu-agent-command qemu: Properly report guest agent errors on command passthrough libvirt-qemu: Dispatch errors from virDomainQemuAgentCommand() src/libvirt-qemu.c | 9 +++++++-- src/qemu/qemu_agent.c | 31 +++++++++++++++++++------------ src/qemu/qemu_driver.c | 10 +++------- tools/virsh-domain.c | 3 +++ 4 files changed, 32 insertions(+), 21 deletions(-) -- 1.8.2.1
Check the returned value for NULL and take the cleanup path appropriately if the API fails. --- tools/virsh-domain.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 46f07ed..9ea5ffc 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7739,7 +7739,10 @@ cmdQemuAgentCommand(vshControl *ctl, const vshCmd *cmd) vshError(ctl, "%s", _("timeout, async and block options are exclusive")); goto cleanup; } + result = virDomainQemuAgentCommand(dom, guest_agent_cmd, timeout, flags); + if (!result) + goto cleanup; if (vshCommandOptBool(cmd, "pretty")) { char *tmp; -- 1.8.2.1
On 06/03/2013 04:34 PM, Peter Krempa wrote:
Check the returned value for NULL and take the cleanup path appropriately if the API fails. --- tools/virsh-domain.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 46f07ed..9ea5ffc 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7739,7 +7739,10 @@ cmdQemuAgentCommand(vshControl *ctl, const vshCmd *cmd) vshError(ctl, "%s", _("timeout, async and block options are exclusive")); goto cleanup; } + result = virDomainQemuAgentCommand(dom, guest_agent_cmd, timeout, flags); + if (!result) + goto cleanup;
if (vshCommandOptBool(cmd, "pretty")) { char *tmp;
ACK, Martin
The code for arbitrary guest agent passthrough was horribly broken since introduciton. Fix it to correctly report errors. --- src/qemu/qemu_agent.c | 31 +++++++++++++++++++------------ src/qemu/qemu_driver.c | 10 +++------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index c7a9681..00fe13f 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1408,25 +1408,32 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon, int timeout) { int ret = -1; - virJSONValuePtr cmd; + virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; *result = NULL; - if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) - return ret; + if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) { + virReportError(VIR_ERR_INVALID_ARG, + _("guest agent timeout '%d' is " + "less than the minimum '%d'"), + timeout, VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN); + goto cleanup; + } - cmd = virJSONValueFromString(cmd_str); - if (!cmd) - return ret; + if (!(cmd = virJSONValueFromString(cmd_str))) + goto cleanup; + + if ((ret = qemuAgentCommand(mon, cmd, &reply, timeout)) < 0) + goto cleanup; - ret = qemuAgentCommand(mon, cmd, &reply, timeout); + if ((ret = qemuAgentCheckError(cmd, reply)) < 0) + goto cleanup; - if (ret == 0) { - ret = qemuAgentCheckError(cmd, reply); - if (!(*result = virJSONValueToString(reply, false))) - ret = -1; - } + if (!(*result = virJSONValueToString(reply, false))) + ret = -1; + +cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); return ret; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5ca0fd4..9d3f632 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14881,16 +14881,12 @@ qemuDomainQemuAgentCommand(virDomainPtr domain, qemuDomainObjEnterAgent(vm); ret = qemuAgentArbitraryCommand(priv->agent, cmd, &result, timeout); qemuDomainObjExitAgent(vm); - if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to execute agent command")); - goto endjob; - } + if (ret < 0) + VIR_FREE(result); endjob: - if (qemuDomainObjEndJob(driver, vm) == 0) { + if (qemuDomainObjEndJob(driver, vm) == 0) vm = NULL; - } cleanup: if (vm) -- 1.8.2.1
On 06/03/2013 04:35 PM, Peter Krempa wrote:
The code for arbitrary guest agent passthrough was horribly broken since introduciton. Fix it to correctly report errors.
s/introduciton/introduction/ ACK, Martin
The original implementation didn't follow the established pattern and did not dispatch errors in case of failure. --- src/libvirt-qemu.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c index 747488d..e884e32 100644 --- a/src/libvirt-qemu.c +++ b/src/libvirt-qemu.c @@ -211,6 +211,7 @@ virDomainQemuAgentCommand(virDomainPtr domain, unsigned int flags) { virConnectPtr conn; + char *ret; VIR_DEBUG("domain=%p, cmd=%s, timeout=%d, flags=%x", domain, cmd, timeout, flags); @@ -228,13 +229,17 @@ virDomainQemuAgentCommand(virDomainPtr domain, conn = domain->conn; if (conn->driver->domainQemuAgentCommand) { - return conn->driver->domainQemuAgentCommand(domain, cmd, - timeout, flags); + ret = conn->driver->domainQemuAgentCommand(domain, cmd, + timeout, flags); + if (!ret) + goto error; + return ret; } virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); /* Copy to connection error object for back compatibility */ +error: virDispatchError(conn); return NULL; } -- 1.8.2.1
On 06/03/2013 04:35 PM, Peter Krempa wrote:
The original implementation didn't follow the established pattern and did not dispatch errors in case of failure. --- src/libvirt-qemu.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c index 747488d..e884e32 100644 --- a/src/libvirt-qemu.c +++ b/src/libvirt-qemu.c @@ -211,6 +211,7 @@ virDomainQemuAgentCommand(virDomainPtr domain, unsigned int flags) { virConnectPtr conn; + char *ret;
VIR_DEBUG("domain=%p, cmd=%s, timeout=%d, flags=%x", domain, cmd, timeout, flags); @@ -228,13 +229,17 @@ virDomainQemuAgentCommand(virDomainPtr domain, conn = domain->conn;
if (conn->driver->domainQemuAgentCommand) { - return conn->driver->domainQemuAgentCommand(domain, cmd, - timeout, flags); + ret = conn->driver->domainQemuAgentCommand(domain, cmd, + timeout, flags); + if (!ret) + goto error; + return ret; }
virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
/* Copy to connection error object for back compatibility */ +error: virDispatchError(conn); return NULL; }
One more fix would fit here, so ACK with this squashed in: diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c index e884e32..9dd76dd 100644 --- a/src/libvirt-qemu.c +++ b/src/libvirt-qemu.c @@ -221,13 +221,14 @@ virDomainQemuAgentCommand(virDomainPtr domain, virDispatchError(NULL); return NULL; } + + conn = domain->conn; + if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(NULL, VIR_ERR_OPERATION_DENIED, __FUNCTION__); - return NULL; + goto error; } - conn = domain->conn; - if (conn->driver->domainQemuAgentCommand) { ret = conn->driver->domainQemuAgentCommand(domain, cmd, timeout, flags); -- Martin
On 06/03/13 17:22, Martin Kletzander wrote:
On 06/03/2013 04:35 PM, Peter Krempa wrote:
The original implementation didn't follow the established pattern and did not dispatch errors in case of failure. --- src/libvirt-qemu.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
One more fix would fit here, so ACK with this squashed in:
I squashed the fix you've proposed and pushed the whole series. Thanks for the review.
Martin
Peter
participants (2)
-
Martin Kletzander -
Peter Krempa