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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list