[libvirt] [PATCH] qemu: make sure agent returns error when required data are missing

[Qiang Guan]

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 at 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