On Sun, Feb 24, 2008 at 10:12:05PM +0000, Daniel P. Berrange wrote:
> On Fri, Feb 22, 2008 at 10:59:43AM -0500, Daniel Veillard wrote:
> > Okay, first patch enclosed, it seems to work for me:
> > + /*
> > + * if running a xen kernel, give it priority over
> > + * QEmu emultation
> > + */
> > + if (STREQ(latest, "xen:///"))
> > + use = latest;
>
> If we edit virInitialize() to make 'xenUnifiedRegister' run before the
> call to 'qemudRegister', then we won't need this check, since the Xen
> driver would get probed ahead of the QEMU driver.
Honnestly I prefer to keep the test in than base the behaviour purely
on the order of the drivers, it exposes the intent clearly, and it's not
like it's a timely critical operation :-)
> > + else if ((use == NULL) && (!STREQ(latest, "test:///")))
> > + use = latest;
>
> IMHO, remove the !STREQ(latest, "test:///") and get rid of the 'probe' impl
> for the test driver. The test driver will always succeed, but should
> only ever be used for test suites, never real world deployment, so
> we shouldn't probe it.
Okay, i wondered if i should keep the test probe or not, and though it would
still be useful if we were to expose the list of drivers from a library API
as you suggested too. But yeah with the current code, let's get rid of it,
[...]
> > +/**
> > + * qemudProbe:
> > + *
> > + * Probe for the availability of the qemu driver, assume the
> > + * presence of QEmu emulation if the binaries are installed
> > + */
> > +static const char *qemudProbe(void)
> > +{
> > + if ((virFileExists("/usr/bin/qemu")) ||
> > + (virFileExists("/usr/bin/qemu-kvm"))) {
> > + if (getuid() == 0) {
> > + return("qemu:///system");
> > + } else {
> > + return("qemu:///session");
> > + }
> > + }
> > + return(NULL);
> > +}
>
> Should add a check for '/usr/bin/xenner' too, which is Gerd's
> Xen compatability layer for the QEMU driver :-)
Okidoc :-)
[...]
> > +static const char *
> > +xenUnifiedProbe (void)
> > +{
> > +#ifdef __linux__
> > + if (virFileExists("/proc/xen"))
> > + return("xen:///");
> > +#endif
> > +#ifdef __sun__
> > + FILE *fh;
> > +
> > + if (fh = fopen(path, "r")) {
> > + fclose(fh);
> > + return("xen:///");
> > + }
> > +#endif
>
> AFAICT this won't compile on Solaris since 'path' isn't defined.
Oops it's fopen("/dev/xen/domcaps", "r") as John suggested,
I will commit with those fixes later today unless someone has an objection,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list