Re: [Libvir] [PATCH] default hypervisor selection
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/