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@xxxxxxxxxx>
> >
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list