[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Search binaries in PATH instead of hardcoding /usr/bin



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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]