[libvirt] [PATCH 4/5] Add a qemuMonitorGetVersion() method for query-version command

[Eric Blake]

On 08/20/2012 07:49 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Add a new qemuMonitorGetVersion() method to support invocation
> of the 'query-version' JSON monitor command. No HMP equivalent
> is provided, since this will only be used for QEMU >= 1.2
> ---
>  src/qemu/qemu_monitor.c      |  24 ++++++++++
>  src/qemu/qemu_monitor.h      |   7 +++
>  src/qemu/qemu_monitor_json.c |  78 +++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_json.h |   7 +++
>  tests/qemumonitorjsontest.c  | 102 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 218 insertions(+)
> 

> +    if (!mon) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("monitor must not be NULL"));
> +        return -1;

Given this error,

> +++ b/src/qemu/qemu_monitor.h
> @@ -574,6 +574,13 @@ int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon,
>  
>  int qemuMonitorSystemWakeup(qemuMonitorPtr mon);
>  
> +int qemuMonitorGetVersion(qemuMonitorPtr mon,
> +                          int *major,
> +                          int *minor,
> +                          int *micro,
> +                          char **package)
> +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);

why not ATTRIBUTE_NONNULL(1) as well?

> +    if (virJSONValueObjectGetNumberInt(qemu, "micro", micro) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-version reply was missing 'micro' version"));
> +        goto cleanup;
> +    }

query-version has existed since 0.14.0, so we are safe using it if we
assume JSON.

Hmm, weren't there versions, like 1.1, which lacked micro?
/me researches...
That's true for the 'qemu -help' output, but it appears that the QMP
command has always provided micro, even when it is 0.  So this should be
safe.

> +    if (package) {
> +        const char *tmp;
> +        if (!(tmp = virJSONValueObjectGetString(data, "package"))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("query-version reply was missing 'package' version"));
> +            goto cleanup;
> +        }

Why is it an error if package is non-NULL but package data was not
present?  Can't we just leave *package=NULL in that case, rather than
erroring out?  After all, when package is NULL, we don't care whether
package data was present.

> +
> +    if (qemuMonitorTestAddItem(test, "query-version",
> +                               "{ "
> +                               "  \"return\":{ "
> +                               "     \"qemu\":{ "
> +                               "        \"major\":0, "
> +                               "        \"minor\":11, "
> +                               "        \"micro\":6 "
> +                               "      },"
> +                               "     \"package\":\"2.283.el6\""
> +                               "  }"
> +                               "}") < 0)

Shouldn't we test typical values in use by actual qemu?  For example,
with RHEL, I see something like "package":"(qemu-kvm-0.12.1.2)".

Overall, I like the idea.  But do we have any code that uses the new
monitor command, besides the testsuite?

-- 
Eric Blake   eblake at 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: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120820/1b5e480b/attachment-0001.sig>