[libvirt] [PATCH] qemu: Search binaries in PATH instead of hardcoding /usr/bin
Daniel Veillard
veillard at redhat.com
Wed Jan 20 15:28:00 UTC 2010
On Wed, Jan 20, 2010 at 02:46:10AM +0100, Matthias Bolte wrote:
> ---
> src/qemu/qemu_conf.c | 137 ++++++++++++++++++++++++++++++--------------------
> 1 files changed, 82 insertions(+), 55 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 92a9348..6141ec6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -379,20 +379,20 @@ static const struct qemu_feature_flags const arch_info_x86_64_flags [] = {
>
> /* The archicture tables for supported QEMU archs */
> static const struct qemu_arch_info const arch_info_hvm[] = {
> - { "i686", 32, NULL, "/usr/bin/qemu",
> - "/usr/bin/qemu-system-x86_64", arch_info_i686_flags, 4 },
> - { "x86_64", 64, NULL, "/usr/bin/qemu-system-x86_64",
> + { "i686", 32, NULL, "qemu",
> + "qemu-system-x86_64", arch_info_i686_flags, 4 },
> + { "x86_64", 64, NULL, "qemu-system-x86_64",
> NULL, arch_info_x86_64_flags, 2 },
> - { "arm", 32, NULL, "/usr/bin/qemu-system-arm", NULL, NULL, 0 },
> - { "mips", 32, NULL, "/usr/bin/qemu-system-mips", NULL, NULL, 0 },
> - { "mipsel", 32, NULL, "/usr/bin/qemu-system-mipsel", NULL, NULL, 0 },
> - { "sparc", 32, NULL, "/usr/bin/qemu-system-sparc", NULL, NULL, 0 },
> - { "ppc", 32, NULL, "/usr/bin/qemu-system-ppc", NULL, NULL, 0 },
> + { "arm", 32, NULL, "qemu-system-arm", NULL, NULL, 0 },
> + { "mips", 32, NULL, "qemu-system-mips", NULL, NULL, 0 },
> + { "mipsel", 32, NULL, "qemu-system-mipsel", NULL, NULL, 0 },
> + { "sparc", 32, NULL, "qemu-system-sparc", NULL, NULL, 0 },
> + { "ppc", 32, NULL, "qemu-system-ppc", NULL, NULL, 0 },
> };
>
> static const struct qemu_arch_info const arch_info_xen[] = {
> - { "i686", 32, "xenner", "/usr/bin/xenner", NULL, arch_info_i686_flags, 4 },
> - { "x86_64", 64, "xenner", "/usr/bin/xenner", NULL, arch_info_x86_64_flags, 2 },
> + { "i686", 32, "xenner", "xenner", NULL, arch_info_i686_flags, 4 },
> + { "x86_64", 64, "xenner", "xenner", NULL, arch_info_x86_64_flags, 2 },
> };
>
>
> @@ -768,8 +768,8 @@ qemudCapsInitGuest(virCapsPtr caps,
> int i;
> int haskvm = 0;
> int haskqemu = 0;
> - const char *kvmbin = NULL;
> - const char *binary = NULL;
> + char *kvmbin = NULL;
> + char *binary = NULL;
> time_t binary_mtime;
> virCapsGuestMachinePtr *machines = NULL;
> int nmachines = 0;
> @@ -779,10 +779,15 @@ qemudCapsInitGuest(virCapsPtr caps,
> /* Check for existance of base emulator, or alternate base
> * which can be used with magic cpu choice
> */
> - if (access(info->binary, X_OK) == 0)
> - binary = info->binary;
> - else if (info->altbinary && access(info->altbinary, X_OK) == 0)
> - binary = info->altbinary;
> + binary = virFindFileInPath(info->binary);
> +
> + if (binary == NULL || access(binary, X_OK) != 0) {
> + VIR_FREE(binary);
> + binary = virFindFileInPath(info->altbinary);
> +
> + if (binary != NULL && access(binary, X_OK) != 0)
> + VIR_FREE(binary);
> + }
>
> /* Can use acceleration for KVM/KQEMU if
> * - host & guest arches match
> @@ -792,16 +797,30 @@ qemudCapsInitGuest(virCapsPtr caps,
> */
> if (STREQ(info->arch, hostmachine) ||
> (STREQ(hostmachine, "x86_64") && STREQ(info->arch, "i686"))) {
> - const char *const kvmbins[] = { "/usr/bin/qemu-kvm", /* Fedora */
> - "/usr/bin/kvm" }; /* Upstream .spec */
> + if (access("/dev/kvm", F_OK) == 0) {
> + const char *const kvmbins[] = { "qemu-kvm", /* Fedora */
> + "kvm" }; /* Upstream .spec */
> +
> + for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) {
> + kvmbin = virFindFileInPath(kvmbins[i]);
> +
> + if (kvmbin == NULL || access(kvmbin, X_OK) != 0) {
> + VIR_FREE(kvmbin);
> + continue;
> + }
>
> - for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) {
> - if (access(kvmbins[i], X_OK) == 0 &&
> - access("/dev/kvm", F_OK) == 0) {
> haskvm = 1;
> - kvmbin = kvmbins[i];
> - if (!binary)
> - binary = kvmbin;
> +
> + if (binary == NULL) {
> + binary = strdup(kvmbin);
> +
> + if (binary == NULL) {
> + virReportOOMError(NULL);
> + VIR_FREE(kvmbin);
> + return -1;
> + }
> + }
> +
> break;
> }
> }
> @@ -826,21 +845,18 @@ qemudCapsInitGuest(virCapsPtr caps,
> virCapsGuestMachinePtr machine;
>
> if (VIR_ALLOC(machine) < 0) {
> - virReportOOMError(NULL);
> - return -1;
> + goto no_memory;
> }
>
> if (!(machine->name = strdup(info->machine))) {
> - virReportOOMError(NULL);
> VIR_FREE(machine);
> - return -1;
> + goto no_memory;
> }
>
> if (VIR_ALLOC_N(machines, 1) < 0) {
> - virReportOOMError(NULL);
> VIR_FREE(machine->name);
> VIR_FREE(machine);
> - return -1;
> + goto no_memory;
> }
>
> machines[0] = machine;
> @@ -853,7 +869,7 @@ qemudCapsInitGuest(virCapsPtr caps,
> old_caps, &machines, &nmachines);
> if (probe &&
> qemudProbeMachineTypes(binary, &machines, &nmachines) < 0)
> - return -1;
> + goto error;
> }
>
> /* We register kvm as the base emulator too, since we can
> @@ -865,21 +881,18 @@ qemudCapsInitGuest(virCapsPtr caps,
> binary,
> NULL,
> nmachines,
> - machines)) == NULL) {
> - for (i = 0; i < nmachines; i++) {
> - VIR_FREE(machines[i]->name);
> - VIR_FREE(machines[i]);
> - }
> - VIR_FREE(machines);
> - return -1;
> - }
> + machines)) == NULL)
> + goto error;
> +
> + machines = NULL;
> + nmachines = 0;
>
> guest->arch.defaultInfo.emulator_mtime = binary_mtime;
>
> if (qemudProbeCPUModels(binary, info->arch, &ncpus, NULL) == 0
> && ncpus > 0
> && !virCapabilitiesAddGuestFeature(guest, "cpuselection", 1, 0))
> - return -1;
> + goto error;
>
> if (hvm) {
> if (virCapabilitiesAddGuestDomain(guest,
> @@ -888,7 +901,7 @@ qemudCapsInitGuest(virCapsPtr caps,
> NULL,
> 0,
> NULL) == NULL)
> - return -1;
> + goto error;
>
> if (haskqemu &&
> virCapabilitiesAddGuestDomain(guest,
> @@ -897,7 +910,7 @@ qemudCapsInitGuest(virCapsPtr caps,
> NULL,
> 0,
> NULL) == NULL)
> - return -1;
> + goto error;
>
> if (haskvm) {
> virCapsGuestDomainPtr dom;
> @@ -911,9 +924,6 @@ qemudCapsInitGuest(virCapsPtr caps,
> binary_mtime = 0;
> }
>
> - machines = NULL;
> - nmachines = 0;
> -
> if (!STREQ(binary, kvmbin)) {
> int probe = 1;
> if (old_caps && binary_mtime)
> @@ -922,7 +932,7 @@ qemudCapsInitGuest(virCapsPtr caps,
> old_caps, &machines, &nmachines);
> if (probe &&
> qemudProbeMachineTypes(kvmbin, &machines, &nmachines) < 0)
> - return -1;
> + goto error;
> }
>
> if ((dom = virCapabilitiesAddGuestDomain(guest,
> @@ -931,14 +941,12 @@ qemudCapsInitGuest(virCapsPtr caps,
> NULL,
> nmachines,
> machines)) == NULL) {
> - for (i = 0; i < nmachines; i++) {
> - VIR_FREE(machines[i]->name);
> - VIR_FREE(machines[i]);
> - }
> - VIR_FREE(machines);
> - return -1;
> + goto error;
> }
>
> + machines = NULL;
> + nmachines = 0;
> +
> dom->info.emulator_mtime = binary_mtime;
> }
> } else {
> @@ -948,7 +956,7 @@ qemudCapsInitGuest(virCapsPtr caps,
> NULL,
> 0,
> NULL) == NULL)
> - return -1;
> + goto error;
> }
>
> if (info->nflags) {
> @@ -957,11 +965,24 @@ qemudCapsInitGuest(virCapsPtr caps,
> info->flags[i].name,
> info->flags[i].default_on,
> info->flags[i].toggle) == NULL)
> - return -1;
> + goto error;
> }
> }
>
> + VIR_FREE(binary);
> + VIR_FREE(kvmbin);
> +
> return 0;
> +
> +no_memory:
> + virReportOOMError(NULL);
> +
> +error:
> + VIR_FREE(binary);
> + VIR_FREE(kvmbin);
> + virCapabilitiesFreeMachines(machines, nmachines);
> +
> + return -1;
> }
>
>
> @@ -1011,6 +1032,7 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
> struct utsname utsname;
> virCapsPtr caps;
> int i;
> + char *xenner = NULL;
>
> /* Really, this never fails - look at the man-page. */
> uname (&utsname);
> @@ -1051,7 +1073,9 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
> goto no_memory;
>
> /* Then possibly the Xen paravirt guests (ie Xenner */
> - if (access("/usr/bin/xenner", X_OK) == 0 &&
> + xenner = virFindFileInPath("xenner");
> +
> + if (xenner != NULL && access(xenner, X_OK) == 0 &&
> access("/dev/kvm", F_OK) == 0) {
> for (i = 0 ; i < ARRAY_CARDINALITY(arch_info_xen) ; i++)
> /* Allow Xen 32-on-32, 32-on-64 and 64-on-64 */
> @@ -1065,12 +1089,15 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
> }
> }
>
> + VIR_FREE(xenner);
> +
> /* QEMU Requires an emulator in the XML */
> virCapabilitiesSetEmulatorRequired(caps);
>
> return caps;
>
> no_memory:
> + VIR_FREE(xenner);
> virCapabilitiesFree(caps);
> return NULL;
> }
Pro: it's more flexible
Cons: it's less rigid
:-)
I think it's fine since in the majority of cases that code will be run
by libvirtd, which as a daemon will have a relatively well defined and
contained PATH . Like when a rogue shared library si available in some
common place that lead to very painful debugging when an user has a
problem, rather than the rather straighforward error about a missing
binary the current version. It's a bit of a double edged sword, but in
that case I think it's fine. But I could see how others could disagree
ACK from my POV,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list