Re: [libvirt] [PATCH 4/5] Add a qemuMonitorGetVersion() method for query-version command
On Mon, Aug 20, 2012 at 10:29:39AM -0600, Eric Blake wrote:
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?
We've had cases in the past where we passed a NULL 'mon' parameter
and the compiler was unable to warn us:
commit 31e29fe5247fd4beca437cdbc49e1b1f30884446
Author: Daniel P. Berrange <berrange(a)redhat.com>
Date: Mon May 17 07:43:36 2010 -0400
Protect against NULL pointer flaws in monitor usage
History has shown that there are frequent bugs in the QEMU driver
code leading to the monitor being invoked with a NULL pointer.
Although the QEMU driver code should always report an error in
this case before invoking the monitor, as a safety net put in a
generic check in the monitor code entry points.
* src/qemu/qemu_monitor.c: Safety net to check for NULL monitor
object
> + 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.
Well I believe this field should be mandatory in the QEMU JSON
schema, so I wanted to treat it that way in libvirt too.
> +
> + 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)".
Oh it was supposed to match, but I guess I messed up
Overall, I like the idea. But do we have any code that uses the new
monitor command, besides the testsuite?
Not yet, I was just sending this code out for early review. I'm working
on a series to stop parsing -help and this is a pre-requisite for it.
Likewise the other commands
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|