[libvirt] [PATCH 5/7] qemu: agent: Make setting of vcpus more robust

[Pavel Hrdina]

On Mon, Jun 20, 2016 at 04:34:19PM +0200, Peter Krempa wrote:
> Documentation for the "guest-set-vcpus" command describes a proper
> algorithm how to set vcpus. This patch makes the following changes:
> 
> - state of cpus that has not changed is not updated
> - if the command was partially successful the command is re-tried with
>   the rest of the arguments to get a proper error message
> - code is more robust against mailicious guest agent

s/mailicious/malicious/

> - fix testsuite to the new semantics
> ---
>  src/qemu/qemu_agent.c  | 83 ++++++++++++++++++++++++++++++++++++++++++--------
>  src/qemu/qemu_agent.h  |  2 ++
>  src/qemu/qemu_driver.c | 13 --------
>  tests/qemuagenttest.c  | 44 ++++++++++++--------------
>  4 files changed, 92 insertions(+), 50 deletions(-)
> 
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index cbc0995..5bd767a 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1523,16 +1523,13 @@ qemuAgentGetVCPUs(qemuAgentPtr mon,
>      return ret;
>  }
> 
> -/**
> - * Set the VCPU state using guest agent.
> - *
> - * Returns -1 on error, ninfo in case everything was successful and less than
> - * ninfo on a partial failure.
> - */
> -int
> -qemuAgentSetVCPUs(qemuAgentPtr mon,
> -                  qemuAgentCPUInfoPtr info,
> -                  size_t ninfo)
> +
> +/* returns the value provided by the guest agent or -1 on internal error */
> +static int
> +qemuAgentSetVCPUsCommand(qemuAgentPtr mon,
> +                         qemuAgentCPUInfoPtr info,
> +                         size_t ninfo,
> +                         int *nmodified)
>  {
>      int ret = -1;
>      virJSONValuePtr cmd = NULL;
> @@ -1541,6 +1538,8 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
>      virJSONValuePtr cpu = NULL;
>      size_t i;
> 
> +    *nmodified = 0;
> +
>      /* create the key data array */
>      if (!(cpus = virJSONValueNewArray()))
>          goto cleanup;
> @@ -1552,6 +1551,12 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
>          if (!(cpu = virJSONValueNewObject()))
>              goto cleanup;
> 
> +        /* don't set state for cpus that were not touched */
> +        if (!in->modified)
> +            continue;

This needs to go before the virJSONValueNewObject, otherwise it would leak
memory.

> +
> +        (*nmodified)++;
> +
>          if (virJSONValueObjectAppendNumberInt(cpu, "logical-id", in->id) < 0)
>              goto cleanup;
> 
> @@ -1564,6 +1569,11 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
>          cpu = NULL;
>      }
> 
> +    if (*nmodified == 0) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
>      if (!(cmd = qemuAgentMakeCommand("guest-set-vcpus",
>                                       "a:vcpus", cpus,
>                                       NULL)))
> @@ -1575,9 +1585,17 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
>                           VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
>          goto cleanup;
> 
> -    if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
> +    if (qemuAgentCheckError(cmd, reply) < 0)
> +        goto cleanup;
> +
> +    /* All negative values are invalid. Return of 0 is bougs since we wouldn't

s/bougs/bogus/

> +     * call the guest agent so that 0 cpus would be set successfully. Reporting
> +     * more successfuly set vcpus that we've asked for is invalid */

s/successfuly/successfully/
s/invalid/invalid./

> +    if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0 ||
> +        ret <= 0 || ret > *nmodified) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("malformed return value"));
> +                       _("guest agent returned malformed or invalid return value"));
> +        ret = -1;
>      }
> 
>   cleanup:
> @@ -1589,6 +1607,45 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
>  }

[...]

ACK