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,