[libvirt] [PATCH] Don't list capabilities entries if emulators can't be accessed
The patch below fixes capabilities xml generation for the qemu driver if the emulators aren't found in the hardcoded paths. Current behavior will add a <guest> entry for the emulator even if it does not exist, patch fixes this to check that we actually have access. This brings up another issue, that hardcoded paths aren't exactly distro friendly. I'm not really familiar with what options we have to allow specifying these at build time (or something else). Anyone have an idea? Thanks, Cole diff --git a/src/qemu_conf.c b/src/qemu_conf.c index dc9e42a..0328cc1 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -230,6 +230,10 @@ qemudCapsInitGuest(virCapsPtr caps, virCapsGuestPtr guest; int i; + /* Check for existance of base emulator */ + if (access(info->binary, X_OK) == -1) + return 0; + if ((guest = virCapabilitiesAddGuest(caps, hvm ? "hvm" : "xen", info->arch, @@ -241,9 +245,7 @@ qemudCapsInitGuest(virCapsPtr caps, return -1; if (hvm) { - /* Check for existance of base emulator */ - if (access(info->binary, X_OK) == 0 && - virCapabilitiesAddGuestDomain(guest, + if (virCapabilitiesAddGuestDomain(guest, "qemu", NULL, NULL, @@ -263,6 +265,7 @@ qemudCapsInitGuest(virCapsPtr caps, return -1; if (access("/dev/kvm", F_OK) == 0 && + access("/usr/bin/qemu-kvm", X_OK) == 0 && virCapabilitiesAddGuestDomain(guest, "kvm", "/usr/bin/qemu-kvm",
On Wed, Aug 20, 2008 at 12:51:05PM -0400, Cole Robinson wrote:
The patch below fixes capabilities xml generation for the qemu driver if the emulators aren't found in the hardcoded paths. Current behavior will add a <guest> entry for the emulator even if it does not exist, patch fixes this to check that we actually have access.
This brings up another issue, that hardcoded paths aren't exactly distro friendly. I'm not really familiar with what options we have to allow specifying these at build time (or something else). Anyone have an idea?
Ideally we'd just list the binary names, and search in $PATH for them. Patches for this welcome...
diff --git a/src/qemu_conf.c b/src/qemu_conf.c index dc9e42a..0328cc1 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -230,6 +230,10 @@ qemudCapsInitGuest(virCapsPtr caps, virCapsGuestPtr guest; int i;
+ /* Check for existance of base emulator */ + if (access(info->binary, X_OK) == -1) + return 0; +
This isn't right - this means that if KVM is installed, but QEMU is not installed you won't get any capabilities. Basically we need todo all the access() checks for QEMU, KVM, /dev/kvm up-front. And then generated the capabilites if either QEMU or KVM is available. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Aug 20, 2008 at 12:51:05PM -0400, Cole Robinson wrote:
The patch below fixes capabilities xml generation for the qemu driver if the emulators aren't found in the hardcoded paths. Current behavior will add a <guest> entry for the emulator even if it does not exist, patch fixes this to check that we actually have access.
This brings up another issue, that hardcoded paths aren't exactly distro friendly. I'm not really familiar with what options we have to allow specifying these at build time (or something else). Anyone have an idea?
Ideally we'd just list the binary names, and search in $PATH for them. Patches for this welcome...
The gnulib "findprog" module can be used to do this, but there are two obstacles: - it's GPL (probably just because... - it exits on an out of memory error However, it's not a big deal to rewrite it not to exit, and make dependent modules will be LGPLv2+. Will require pulling the mfile_name_cat function out into its own module. I'm working on this.
diff --git a/src/qemu_conf.c b/src/qemu_conf.c index dc9e42a..0328cc1 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -230,6 +230,10 @@ qemudCapsInitGuest(virCapsPtr caps, virCapsGuestPtr guest; int i;
+ /* Check for existance of base emulator */ + if (access(info->binary, X_OK) == -1) + return 0; +
This isn't right - this means that if KVM is installed, but QEMU is not installed you won't get any capabilities.
Basically we need todo all the access() checks for QEMU, KVM, /dev/kvm up-front. And then generated the capabilites if either QEMU or KVM is available.
Daniel
Daniel P. Berrange wrote:
On Wed, Aug 20, 2008 at 12:51:05PM -0400, Cole Robinson wrote:
diff --git a/src/qemu_conf.c b/src/qemu_conf.c index dc9e42a..0328cc1 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -230,6 +230,10 @@ qemudCapsInitGuest(virCapsPtr caps, virCapsGuestPtr guest; int i;
+ /* Check for existance of base emulator */ + if (access(info->binary, X_OK) == -1) + return 0; +
This isn't right - this means that if KVM is installed, but QEMU is not installed you won't get any capabilities.
Basically we need todo all the access() checks for QEMU, KVM, /dev/kvm up-front. And then generated the capabilites if either QEMU or KVM is available.
Okay, I think this patch solves the issues. We check upfront for the base emulator and potential kvm emulators (qemu-kvm, and /usr/bin/kvm for ubuntu/upstream .spec). If nothing is found, just return. If only the base emulator is found, skip kvm even if /dev/kvm exists. If only kvm bin is found, add the base emulator capabilities only if emulator and host arch matches. Thanks, Cole diff --git a/src/qemu_conf.c b/src/qemu_conf.c index ee5851d..4ef75dd 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -226,7 +226,29 @@ qemudCapsInitGuest(virCapsPtr caps, const struct qemu_arch_info *info, int hvm) { virCapsGuestPtr guest; - int i; + int i, haskvm, hasbase, samearch; + const char *kvmbin = NULL; + + /* Check for existance of base emulator */ + hasbase = (access(info->binary, X_OK) == 0); + + samearch = STREQ(info->arch, hostmachine); + if (samearch) { + const char *const kvmbins[] = { "/usr/bin/qemu-kvm", /* Fedora */ + "/usr/bin/kvm" }; /* Upstream .spec */ + + for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) { + if ((haskvm = (access(kvmbins[i], X_OK) == 0))) { + kvmbin = kvmbins[i]; + break; + } + } + } else { + haskvm = 0; + } + + if (!hasbase && !haskvm) + return 0; if ((guest = virCapabilitiesAddGuest(caps, hvm ? "hvm" : "xen", @@ -239,8 +261,7 @@ qemudCapsInitGuest(virCapsPtr caps, return -1; if (hvm) { - /* Check for existance of base emulator */ - if (access(info->binary, X_OK) == 0 && + if (hasbase && virCapabilitiesAddGuestDomain(guest, "qemu", NULL, @@ -250,7 +271,7 @@ qemudCapsInitGuest(virCapsPtr caps, return -1; /* If guest & host match, then we can accelerate */ - if (STREQ(info->arch, hostmachine)) { + if (samearch) { if (access("/dev/kqemu", F_OK) == 0 && virCapabilitiesAddGuestDomain(guest, "kqemu", @@ -261,9 +282,10 @@ qemudCapsInitGuest(virCapsPtr caps, return -1; if (access("/dev/kvm", F_OK) == 0 && + haskvm && virCapabilitiesAddGuestDomain(guest, "kvm", - "/usr/bin/qemu-kvm", + kvmbin, NULL, 0, NULL) == NULL)
On Mon, Aug 25, 2008 at 01:16:39PM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
On Wed, Aug 20, 2008 at 12:51:05PM -0400, Cole Robinson wrote:
diff --git a/src/qemu_conf.c b/src/qemu_conf.c index dc9e42a..0328cc1 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -230,6 +230,10 @@ qemudCapsInitGuest(virCapsPtr caps, virCapsGuestPtr guest; int i;
+ /* Check for existance of base emulator */ + if (access(info->binary, X_OK) == -1) + return 0; +
This isn't right - this means that if KVM is installed, but QEMU is not installed you won't get any capabilities.
Basically we need todo all the access() checks for QEMU, KVM, /dev/kvm up-front. And then generated the capabilites if either QEMU or KVM is available.
Okay, I think this patch solves the issues. We check upfront for the base emulator and potential kvm emulators (qemu-kvm, and /usr/bin/kvm for ubuntu/upstream .spec). If nothing is found, just return. If only the base emulator is found, skip kvm even if /dev/kvm exists. If only kvm bin is found, add the base emulator capabilities only if emulator and host arch matches.
ACK, this looks good now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Aug 25, 2008 at 01:16:39PM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
This isn't right - this means that if KVM is installed, but QEMU is not installed you won't get any capabilities.
Basically we need todo all the access() checks for QEMU, KVM, /dev/kvm up-front. And then generated the capabilites if either QEMU or KVM is available.
Okay, I think this patch solves the issues. We check upfront for the base emulator and potential kvm emulators (qemu-kvm, and /usr/bin/kvm for ubuntu/upstream .spec). If nothing is found, just return. If only the base emulator is found, skip kvm even if /dev/kvm exists. If only kvm bin is found, add the base emulator capabilities only if emulator and host arch matches.
That looks fine, applied and commited to CVS, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering