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

On Tue, Sep 25, 2012 at 19:00:13 +0100, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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.

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

> @@ -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.

> +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.

> + > + 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.