[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



2010/1/22 Daniel P. Berrange <berrange redhat com>:
> 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
From 9119b95aeddd2171341e1c996e9ebd26f42fec30 Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias bolte googlemail com>
Date: Wed, 20 Jan 2010 00:24:47 +0100
Subject: [PATCH] qemu: Search binaries in PATH instead of hardcoding /usr/bin

---
 src/qemu/qemu_conf.c |  134 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 80 insertions(+), 54 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c227fe1..f4a6c08 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -384,20 +384,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 },
 };
 
 
@@ -773,21 +773,27 @@ 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;
     struct stat st;
     unsigned int ncpus;
+    int ret = -1;
 
     /* 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
@@ -797,16 +803,22 @@ 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;
+
                 break;
             }
         }
@@ -831,23 +843,20 @@ 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;
         }
 
         nmachines = 1;
 
         if (VIR_ALLOC_N(machines, nmachines) < 0) {
-            virReportOOMError(NULL);
             VIR_FREE(machine->name);
             VIR_FREE(machine);
-            return -1;
+            goto no_memory;
         }
 
         machines[0] = machine;
@@ -859,7 +868,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
@@ -871,21 +880,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,
@@ -894,7 +900,7 @@ qemudCapsInitGuest(virCapsPtr caps,
                                           NULL,
                                           0,
                                           NULL) == NULL)
-            return -1;
+            goto error;
 
         if (haskqemu &&
             virCapabilitiesAddGuestDomain(guest,
@@ -903,7 +909,7 @@ qemudCapsInitGuest(virCapsPtr caps,
                                           NULL,
                                           0,
                                           NULL) == NULL)
-            return -1;
+            goto error;
 
         if (haskvm) {
             virCapsGuestDomainPtr dom;
@@ -917,9 +923,6 @@ qemudCapsInitGuest(virCapsPtr caps,
                 binary_mtime = 0;
             }
 
-            machines = NULL;
-            nmachines = 0;
-
             if (!STREQ(binary, kvmbin)) {
                 int probe = 1;
                 if (old_caps && binary_mtime)
@@ -928,7 +931,7 @@ qemudCapsInitGuest(virCapsPtr caps,
                                                  old_caps, &machines, &nmachines);
                 if (probe &&
                     qemudProbeMachineTypes(kvmbin, &machines, &nmachines) < 0)
-                    return -1;
+                    goto error;
             }
 
             if ((dom = virCapabilitiesAddGuestDomain(guest,
@@ -937,14 +940,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 {
@@ -954,7 +955,7 @@ qemudCapsInitGuest(virCapsPtr caps,
                                           NULL,
                                           0,
                                           NULL) == NULL)
-            return -1;
+            goto error;
     }
 
     if (info->nflags) {
@@ -963,11 +964,30 @@ qemudCapsInitGuest(virCapsPtr caps,
                                                info->flags[i].name,
                                                info->flags[i].default_on,
                                                info->flags[i].toggle) == NULL)
-                return -1;
+                goto error;
         }
     }
 
-    return 0;
+    ret = 0;
+
+cleanup:
+    if (binary == kvmbin) {
+        /* don't double free */
+        VIR_FREE(binary);
+    } else {
+        VIR_FREE(binary);
+        VIR_FREE(kvmbin);
+    }
+
+    return ret;
+
+no_memory:
+    virReportOOMError(NULL);
+
+error:
+    virCapabilitiesFreeMachines(machines, nmachines);
+
+    goto cleanup;
 }
 
 
@@ -1017,6 +1037,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);
@@ -1057,7 +1078,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 */
@@ -1071,12 +1094,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;
 }
-- 
1.6.3.3


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