Re: [libvirt] [PATCH 4/5] Add a qemuMonitorGetVersion() method for query-version command
On 08/20/2012 07:49 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)