[libvirt] [PATCH 4/7] qemu_agent: Introduce helpers for agent based CPU hot(un)plug

[Eric Blake]

On 04/15/2013 09:11 AM, Peter Krempa wrote:
> The qemu guest agent allows to online and offline CPUs from the perspective of
> the guest. This patch adds helpers that call 'guest-get-vcpus' and
> 'guest-set-vcpus' guest agent functions and convert the data for internal
> libvirt usage.
> ---
>  src/qemu/qemu_agent.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_agent.h |  12 +++++
>  2 files changed, 149 insertions(+)
> 
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c

> +int
> +qemuAgentGetVCPUs(qemuAgentPtr mon,
> +                  qemuAgentCPUInfoPtr *info)
> +{
> +    int ret = -1;
> +    int i;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr reply = NULL;
> +    virJSONValuePtr data = NULL;
> +
> +    if (!(cmd = qemuAgentMakeCommand("guest-get-vcpus", NULL)))
> +        return -1;
> +
> +    if (qemuAgentCommand(mon, cmd, &reply,
> +                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 ||
> +        qemuAgentCheckError(cmd, reply) < 0)
> +        goto cleanup;
> +
> +    if (!(data = virJSONValueObjectGet(reply, "return"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("guest-get-vcpus reply was missing return data"));
> +        goto cleanup;
> +    }
> +
> +    if (data->type != VIR_JSON_TYPE_ARRAY) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("guest-get-vcpus return information was not an array"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC_N(*info, virJSONValueArraySize(data)) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    for (i = 0 ; i < virJSONValueArraySize(data) ; i++) {

Quite a few calls to this...

> +        virJSONValuePtr entry = virJSONValueArrayGet(data, i);
> +        qemuAgentCPUInfoPtr in = *info + i;
> +
> +        if (!entry) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("array element missing in guest-get-vcpus return "
> +                             "value"));
> +            goto cleanup;
> +        }
> +
> +        if (virJSONValueObjectGetNumberUint(entry, "logical-id", &in->id) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("'logical-id' missing in reply of guest-get-vcpus"));
> +            goto cleanup;
> +        }
> +
> +        if (virJSONValueObjectGetBoolean(entry, "online", &in->online) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("'online' missing in reply of guest-get-vcpus "));

trailing space in the error message

> +            goto cleanup;
> +        }
> +
> +        if (virJSONValueObjectGetBoolean(entry, "can-offline",
> +                                         &in->offlinable) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("'can-offline' missing in reply of guest-get-vcpus"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = virJSONValueArraySize(data);

...and again.  Worth caching in a local variable?

> +
> +cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    virJSONValueFree(data);
> +    return ret;
> +}

I checked qemu's qga/qapi-schema.json, and this matches the documentation.

> +
> +int
> +qemuAgentSetVCPUs(qemuAgentPtr mon,
> +                  qemuAgentCPUInfoPtr info,
> +                  size_t ninfo)
> +{
> +    int ret = -1;
> +    virJSONValuePtr cmd = NULL;
> +    virJSONValuePtr reply = NULL;
> +    virJSONValuePtr cpus = NULL;
> +    virJSONValuePtr cpu = NULL;
> +    size_t i;
> +
> +    /* create the key data array */
> +    if (!(cpus = virJSONValueNewArray()))
> +        goto no_memory;
> +
> +    for (i = 0; i < ninfo; i++) {
> +        qemuAgentCPUInfoPtr in = &info[i];
> +
> +        /* create single cpu object */
> +        if (!(cpu = virJSONValueNewObject()))
> +            goto no_memory;
> +
> +        if (virJSONValueObjectAppendNumberInt(cpu, "logical-id", in->id) < 0)
> +            goto no_memory;
> +
> +        if (virJSONValueObjectAppendBoolean(cpu, "online", in->online) < 0)
> +            goto no_memory;
> +
> +        if (virJSONValueArrayAppend(cpus, cpu) < 0)
> +            goto no_memory;
> +
> +        cpu = NULL;
> +    }
> +
> +    if (!(cmd = qemuAgentMakeCommand("guest-set-vcpus",
> +                                     "a:vcpus", cpus,
> +                                     NULL)))
> +        goto cleanup;

This has transfer semantics - after this point, cpus is owned by cmd.
You need to do:
 cpus = NULL
here...

> +
> +    if (qemuAgentCommand(mon, cmd, &reply,
> +                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 ||
> +        qemuAgentCheckError(cmd, reply) < 0)
> +        goto cleanup;
> +
> +    if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("malformed return value"));
> +    }

Potential problem.  In the qga documentation, this function is
documented as returning < ninfo if the command partially succeeded.  I
don't know how you plan on using this in later functions, but it might
be worth documenting that this function has the same read()-like
semantics (a return of -1 is outright error, a return of 0 meant no
changes were requested, a return of < ninfo means the first few values
were processed before hitting an error, and a return of ninfo means all
changes were successful).

> +
> +cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    virJSONValueFree(cpu);
> +    virJSONValueFree(cpus);

...in order to avoid a double-free here.

> +    return ret;
> +
> +no_memory:
> +    virReportOOMError();
> +    goto cleanup;
> +}
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index ba04e61..79be283 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -82,4 +82,16 @@ int qemuAgentArbitraryCommand(qemuAgentPtr mon,
>                                int timeout);
>  int qemuAgentFSTrim(qemuAgentPtr mon,
>                      unsigned long long minimum);
> +
> +
> +typedef struct _qemuAgentCPUInfo qemuAgentCPUInfo;
> +typedef qemuAgentCPUInfo *qemuAgentCPUInfoPtr;
> +struct _qemuAgentCPUInfo {
> +    unsigned int id;    /* locigal cpu ID */

s/locigal/logical/

> +    bool online;        /* true if the CPU is activated */
> +    bool offlinable;    /* true if the CPU can be offlined - ignored when setting*/

space before */, and maybe wrap this long line

Enough comments that it is probably worth seeing a v2 (or at least
waiting for my review of the client of this new function in the tail of
the series).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130415/e712d6e8/attachment-0001.sig>