[libvirt] [PATCH] qemu: make sure agent returns error when required data are missing
Commit 5b3492fa aimed to fix this and caught one error but exposed another one. When agent command is being executed and the thread waiting for the reply is woken up by an event (e.g. EOF in case of shutdown), the command finishes with no data (rxObject == NULL), but no error is reported, since this might be desired by the caller (e.g. suspend through agent). However, in other situations, when the data are required (e.g. getting vCPUs), we proceed to getting desired data out of the reply, but none of the virJSON*() functions works well with NULLs. I chose the way of a new parameter for qemuAgentCommand() function that specifies whether reply is required and behaves according to that. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1058149 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Notes: This issue probably exists since qemu-ga is supported in libvirt, so this (along with 5b3492fa and e9d09fe1) might be worth back-porting to some maintenance branches, I just haven't gone through them to see which ones are applicable. src/qemu/qemu_agent.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 92573bd..4082331 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1080,6 +1080,7 @@ static int qemuAgentCommand(qemuAgentPtr mon, virJSONValuePtr cmd, virJSONValuePtr *reply, + bool needReply, int seconds) { int ret = -1; @@ -1111,7 +1112,7 @@ qemuAgentCommand(qemuAgentPtr mon, /* If we haven't obtained any reply but we wait for an * event, then don't report this as error */ if (!msg.rxObject) { - if (await_event) { + if (await_event && !needReply) { VIR_DEBUG("Woken up by event %d", await_event); } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1275,7 +1276,7 @@ int qemuAgentShutdown(qemuAgentPtr mon, mon->await_event = QEMU_AGENT_EVENT_RESET; else mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN; - ret = qemuAgentCommand(mon, cmd, &reply, + ret = qemuAgentCommand(mon, cmd, &reply, false, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK); virJSONValueFree(cmd); @@ -1305,7 +1306,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon) if (!cmd) return -1; - if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup; @@ -1342,7 +1343,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon) if (!cmd) return -1; - if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup; @@ -1379,7 +1380,7 @@ qemuAgentSuspend(qemuAgentPtr mon, return -1; mon->await_event = QEMU_AGENT_EVENT_SUSPEND; - ret = qemuAgentCommand(mon, cmd, &reply, + ret = qemuAgentCommand(mon, cmd, &reply, false, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK); virJSONValueFree(cmd); @@ -1409,7 +1410,7 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon, if (!(cmd = virJSONValueFromString(cmd_str))) goto cleanup; - if ((ret = qemuAgentCommand(mon, cmd, &reply, timeout)) < 0) + if ((ret = qemuAgentCommand(mon, cmd, &reply, true, timeout)) < 0) goto cleanup; if (!(*result = virJSONValueToString(reply, false))) @@ -1436,7 +1437,7 @@ qemuAgentFSTrim(qemuAgentPtr mon, if (!cmd) return ret; - ret = qemuAgentCommand(mon, cmd, &reply, + ret = qemuAgentCommand(mon, cmd, &reply, false, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK); virJSONValueFree(cmd); @@ -1458,7 +1459,7 @@ qemuAgentGetVCPUs(qemuAgentPtr mon, if (!(cmd = qemuAgentMakeCommand("guest-get-vcpus", NULL))) return -1; - if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup; @@ -1566,7 +1567,7 @@ qemuAgentSetVCPUs(qemuAgentPtr mon, cpus = NULL; - if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup; -- 1.9.1
On Apr 3, 2014 at 13:58PM, Martin Kletzander wrote:
Commit 5b3492fa aimed to fix this and caught one error but exposed another one. When agent command is being executed and the thread waiting for the reply is woken up by an event (e.g. EOF in case of shutdown), the command finishes with no data (rxObject == NULL), but no error is reported, since this might be desired by the caller (e.g. suspend through agent). However, in other situations, when the data are required (e.g. getting vCPUs), we proceed to getting desired data out of the reply, but none of the virJSON*() functions works well with NULLs. I chose the way of a new parameter for qemuAgentCommand() function that specifies whether reply is required and behaves according to that.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1058149
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: This issue probably exists since qemu-ga is supported in libvirt, so this (along with 5b3492fa and e9d09fe1) might be worth back-porting to some maintenance branches, I just haven't gone through them to see which ones are applicable.
src/qemu/qemu_agent.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 92573bd..4082331 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1080,6 +1080,7 @@ static int qemuAgentCommand(qemuAgentPtr mon, virJSONValuePtr cmd, virJSONValuePtr *reply, + bool needReply, int seconds) { int ret = -1; @@ -1111,7 +1112,7 @@ qemuAgentCommand(qemuAgentPtr mon, /* If we haven't obtained any reply but we wait for an * event, then don't report this as error */ if (!msg.rxObject) { - if (await_event) { + if (await_event && !needReply) { VIR_DEBUG("Woken up by event %d", await_event); } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1275,7 +1276,7 @@ int qemuAgentShutdown(qemuAgentPtr mon, mon->await_event = QEMU_AGENT_EVENT_RESET; else mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN; - ret = qemuAgentCommand(mon, cmd, &reply, + ret = qemuAgentCommand(mon, cmd, &reply, false, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
virJSONValueFree(cmd); @@ -1305,7 +1306,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon) if (!cmd) return -1;
- if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup;
@@ -1342,7 +1343,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon) if (!cmd) return -1;
- if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup;
@@ -1379,7 +1380,7 @@ qemuAgentSuspend(qemuAgentPtr mon, return -1;
mon->await_event = QEMU_AGENT_EVENT_SUSPEND; - ret = qemuAgentCommand(mon, cmd, &reply, + ret = qemuAgentCommand(mon, cmd, &reply, false, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
virJSONValueFree(cmd); @@ -1409,7 +1410,7 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon, if (!(cmd = virJSONValueFromString(cmd_str))) goto cleanup;
- if ((ret = qemuAgentCommand(mon, cmd, &reply, timeout)) < 0) + if ((ret = qemuAgentCommand(mon, cmd, &reply, true, timeout)) < 0) goto cleanup;
if (!(*result = virJSONValueToString(reply, false))) @@ -1436,7 +1437,7 @@ qemuAgentFSTrim(qemuAgentPtr mon, if (!cmd) return ret;
- ret = qemuAgentCommand(mon, cmd, &reply, + ret = qemuAgentCommand(mon, cmd, &reply, false, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
virJSONValueFree(cmd); @@ -1458,7 +1459,7 @@ qemuAgentGetVCPUs(qemuAgentPtr mon, if (!(cmd = qemuAgentMakeCommand("guest-get-vcpus", NULL))) return -1;
- if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup;
@@ -1566,7 +1567,7 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
cpus = NULL;
- if (qemuAgentCommand(mon, cmd, &reply, + if (qemuAgentCommand(mon, cmd, &reply, true, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) goto cleanup;
From code logic, this patch seems to be the right answer for bug 1058149. To verify it, I'll do a test with my bug reproducing scripts. Thanks for Martin's help about this problem.
-- ------------ Jackie Best Regards
On 04/03/14 07:58, Martin Kletzander wrote:
Commit 5b3492fa aimed to fix this and caught one error but exposed another one. When agent command is being executed and the thread waiting for the reply is woken up by an event (e.g. EOF in case of shutdown), the command finishes with no data (rxObject == NULL), but no error is reported, since this might be desired by the caller (e.g. suspend through agent). However, in other situations, when the data are required (e.g. getting vCPUs), we proceed to getting desired data out of the reply, but none of the virJSON*() functions works well with NULLs. I chose the way of a new parameter for qemuAgentCommand() function that specifies whether reply is required and behaves according to that.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1058149
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: This issue probably exists since qemu-ga is supported in libvirt, so this (along with 5b3492fa and e9d09fe1) might be worth back-porting to some maintenance branches, I just haven't gone through them to see which ones are applicable.
src/qemu/qemu_agent.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 92573bd..4082331 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1080,6 +1080,7 @@ static int qemuAgentCommand(qemuAgentPtr mon, virJSONValuePtr cmd, virJSONValuePtr *reply, + bool needReply, int seconds) {
Seeing that the "needReply" parameter is true in most cases, I'd rename this function and add one not requiring the new parameter for normal usage and use the renamed in the few cases that actually expect the VM to quit. But this is just a cosmetic issue. ACK Peter
On Thu, Apr 03, 2014 at 09:32:02AM +0200, Peter Krempa wrote:
On 04/03/14 07:58, Martin Kletzander wrote:
Commit 5b3492fa aimed to fix this and caught one error but exposed another one. When agent command is being executed and the thread waiting for the reply is woken up by an event (e.g. EOF in case of shutdown), the command finishes with no data (rxObject == NULL), but no error is reported, since this might be desired by the caller (e.g. suspend through agent). However, in other situations, when the data are required (e.g. getting vCPUs), we proceed to getting desired data out of the reply, but none of the virJSON*() functions works well with NULLs. I chose the way of a new parameter for qemuAgentCommand() function that specifies whether reply is required and behaves according to that.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1058149
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: This issue probably exists since qemu-ga is supported in libvirt, so this (along with 5b3492fa and e9d09fe1) might be worth back-porting to some maintenance branches, I just haven't gone through them to see which ones are applicable.
src/qemu/qemu_agent.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 92573bd..4082331 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1080,6 +1080,7 @@ static int qemuAgentCommand(qemuAgentPtr mon, virJSONValuePtr cmd, virJSONValuePtr *reply, + bool needReply, int seconds) {
Seeing that the "needReply" parameter is true in most cases, I'd rename this function and add one not requiring the new parameter for normal usage and use the renamed in the few cases that actually expect the VM to quit.
I don't think it's worth adding this much of unnecessary code just to eliminate one argument in 4 out of 7 call, especially when all the functions are private and even static, therefore not much of a cleanup from the readability POV.
But this is just a cosmetic issue.
Patches are welcome, though ;)
ACK
Thanks, pushed
Peter
Martin
participants (3)
-
Martin Kletzander -
Peter Krempa -
Qiang Guan