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

@@ -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; }

- 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; }

2010/1/22 Daniel P. Berrange <berrange(a)redhat.com&gt;: > On Wed, Jan 20, 2010 at 02:46:10AM +0100, Matthias Bolte wrote: >> @@ -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); > > Why does this need to strdup(kvmbin), when virFindFileInPath() is > already returning a newly allocated string ?  Seems like we could > just avoid that The common case is binary pointing to a QEMU binary and if KVM could be used and is available kvmbin points to a KVM enabled QEMU binary. So the paths a different in the common case. In the cleanup section both strings need to be freed. The special case is if binary is NULL but we find a KVM binary, then binary is strdup'ed from kvmbin. Before this patch binary and kvmbin were stack allocated and binary = kvmbin was okay. Now binary = kvmbin would result in a double free with    VIR_FREE(binary);    VIR_FREE(kvmbin); But you're right, I just thought about it and    binary = strdup(kvmbin); could be changed back to    binary = kvmbin; and the cleanup code could be changed to    if (binary == kvmbin) {        /* don't double free */        VIR_FREE(binary);    } else {        VIR_FREE(binary);        VIR_FREE(kvmbin);    } to avoid a double free. >> + >> +                    if (binary == NULL) { >> +                        virReportOOMError(NULL); >> +                        VIR_FREE(kvmbin); >> +                        return -1; >> +                    } >> +                } >> + >>                  break; >>              } > > I think this loop is also leaking 'kvmbin', since it just > overrwrites 'kvmbin' on each iteration, the final cleanup > code will only free the last one that was allocated No leak here. The for loop can be left at 3 points: - continue; if binary not found or not executable - return -1; on OOM error - break; on success In both negative cases kvmbin is freed. >> -                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; >>  } >> > > > Generally the patch looks OK though > > Daniel > Version 2 of the patch is attached. Matthias