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

[Daniel P. Berrange]

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 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?

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 at 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 :|