[libvirt] [PATCH v3 20/20] Add support for detecting capablities using QMP commands

[Daniel P. Berrange]

On Thu, Sep 27, 2012 at 03:00:14PM +0200, Jiri Denemark wrote:
> On Tue, Sep 25, 2012 at 19:00:13 +0100, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > Start a QEMU process using
> > 
> >    $QEMU -S -no-user-config -nodefconfig -nodefaults \
> >          -nographic -M none -qmp stdio
> 
> -nodefconfig should not ever be used if QEMU supports -no-user-config. The
> reason is that -nodefconfig disables loading of all files even those that
> reside somewhere in /usr/share and may contain required data, such as CPU
> definitions (although they were moved back to the code recently) or machine
> type definitions if they ever implement the ideas to separate them from the
> code.

Hmm, yes, I forgot about that.
> > @@ -1282,6 +1285,7 @@ static struct qemuCapsStringFlags qemuCapsObjectPropsVirtioNet[] = {
> >  };
> >  
> >  static struct qemuCapsStringFlags qemuCapsObjectPropsPciAssign[] = {
> > +    { "rombar", QEMU_CAPS_PCI_ROMBAR },
> >      { "configfd", QEMU_CAPS_PCI_CONFIGFD },
> >      { "bootindex", QEMU_CAPS_PCI_BOOTINDEX },
> >  };
> > @@ -1857,6 +1861,10 @@ qemuCapsProbeQMPCommands(qemuCapsPtr caps,
> >              qemuCapsSet(caps, QEMU_CAPS_BLOCKJOB_ASYNC);
> >          else if (STREQ(name, "dump-guest-memory"))
> >              qemuCapsSet(caps, QEMU_CAPS_DUMP_GUEST_MEMORY);
> > +        else if (STREQ(name, "query-spice"))
> > +            qemuCapsSet(caps, QEMU_CAPS_SPICE);
> > +        else if (STREQ(name, "query-kvm"))
> > +            qemuCapsSet(caps, QEMU_CAPS_KVM);
> >          VIR_FREE(name);
> >      }
> >      VIR_FREE(commands);
> 
> Hmm, looks like these two hunks should rather go into the previous patch,
> shouldn't they?

No, the previous patch was just refactoring existing code. The existing
approach to detecting spice/kvm was to use -help parsing. Only with the
new QMP code in this patch, do we now detect it via the QMP command list


> > @@ -1979,18 +2078,252 @@ qemuCapsPtr qemuCapsNewForBinary(const char *binary)
> >       * understands the 0.13.0+ notion of "-device driver,".  */
> >      if (qemuCapsGet(caps, QEMU_CAPS_DEVICE) &&
> >          strstr(help, "-device driver,?") &&
> > -        qemuCapsExtractDeviceStr(binary, caps) < 0)
> > -        goto error;
> > +        qemuCapsExtractDeviceStr(caps->binary, caps) < 0)
> > +        goto cleanup;
> >  
> >      if (qemuCapsProbeCPUModels(caps) < 0)
> > -        goto error;
> > +        goto cleanup;
> >  
> >      if (qemuCapsProbeMachineTypes(caps) < 0)
> > -        goto error;
> > +        goto cleanup;
> > +
> > +    ret = 0;
> > +cleanup:
> > +    virCommandFree(cmd);
> > +    return ret;
> > +}
> 
> I think you actually didn't want to remove VIR_FREE(help); from the cleanup
> section.

Yeah, bug.

> > +static int
> > +qemuCapsInitQMP(qemuCapsPtr caps)
> > +{
> > +    int ret = -1;
> > +    virCommandPtr cmd = NULL;
> > +    virDomainObjPtr vm = NULL;
> > +    int socks[2] = { -1, -1 };
> > +    qemuMonitorPtr mon = NULL;
> > +    int major, minor, micro;
> > +    char *package;
> > +
> > +    VIR_DEBUG("caps=%p", caps);
> > +
> > +    if (socketpair(PF_UNIX, SOCK_STREAM, 0, socks) < 0) {
> > +        virReportSystemError(errno, "%s",
> > +                             _("unable to create socket pair"));
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!(vm = virDomainObjNew(NULL)))
> > +        goto cleanup;
> 
> Heh, AFAICT, vm in qemuMonitorOpenFD is only used to fill in mon->vm so that
> the VM pointer can be passed to various callbacks when something asynchronous
> happens in the monitor. Looks like we can just safely pass NULL to
> qemuMonitorOpenFD() instead.

There is one place in the monitor which uses it, but we can avoid it.

> 
> > +
> > +    cmd = virCommandNewArgList(caps->binary,
> > +                               "-S",
> > +                               "-no-user-config",
> > +                               "-nodefconfig",
> 
> Remove the -nodefconfig parameter, since this code requires at least qemu-1.2
> and thus it will either support -no-user-config or be unusable anyway.
> 
> > +                               "-nodefaults",
> > +                               "-nographic",
> > +                               "-M", "none",
> > +                               "-qmp", "stdio",
> > +                               NULL);
> > +    virCommandAddEnvPassCommon(cmd);
> > +    virCommandSetInputFD(cmd, socks[1]);
> > +    virCommandSetOutputFD(cmd, &socks[1]);
> > +    virCommandClearCaps(cmd);
> > +
> > +    if (virCommandRunAsync(cmd, NULL) < 0)
> > +        goto cleanup;
> 
> However, if qemu is not new enough to support -no-user-config, virCommand will
> fail and this would consider it a fatal error instead of falling back to help
> parsing.

Actually not, because we're running async here, so we don't detect the
failed QEMU at this point, only later in this method. There is a problem
here though - my current code will cause log messages to be generated
when hitting old QEMU which is undesirable. So I'm changing this to use
virCommandRun and -daemonize QEMU, so we can detect exit status for old
QEMU without polluting the log.

Enough changes that I'll re-post this.

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