Various fixes following a code review

* src/libvirt.c src/lxc/lxc_conf.c src/lxc/lxc_container.c
  src/lxc/lxc_controller.c src/node_device/node_device_hal.c
  src/openvz/openvz_conf.c src/qemu/qemu_driver.c
  src/qemu/qemu_monitor_text.c src/remote/remote_driver.c
  src/storage/storage_backend_disk.c src/storage/storage_driver.c
  src/util/logging.c src/xen/sexpr.c src/xen/xend_internal.c
  src/xen/xm_internal.c: Steve Grubb <sgrubb@redhat.com> sent a code
  review and those are the fixes correcting the problems

diff --git a/src/libvirt.c b/src/libvirt.c
index 2c5079088ce5dccb81ba2cea215938a94dacfbbb..9e80e29ef74c936005066dc5f4e3fc11abac5246 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1122,7 +1122,7 @@ do_open (const char *name,
               (res == VIR_DRV_OPEN_DECLINED ? "DECLINED" :
                (res == VIR_DRV_OPEN_ERROR ? "ERROR" : "unknown status")));
         if (res == VIR_DRV_OPEN_ERROR) {
-            if (0 && STREQ(virStorageDriverTab[i]->name, "remote")) {
+            if (STREQ(virStorageDriverTab[i]->name, "remote")) {
                 virLibConnWarning (NULL, VIR_WAR_NO_STORAGE,
                                    "Is the daemon running ?");
             }

diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c
index 74dc3674765130e35114eedd3b7294188bc74c86..bc815432e5bf032240774d3608707ccf843afa54 100644 (file)
--- a/src/lxc/lxc_conf.c
+++ b/src/lxc/lxc_conf.c
@@ -31,6 +31,7 @@
 #include "nodeinfo.h"
 #include "virterror_internal.h"
 #include "conf.h"
+#include "memory.h"
 #include "logging.h"
 
 
@@ -111,10 +112,10 @@ int lxcLoadDriverConfig(lxc_driver_t *driver)
 
     /* Avoid error from non-existant or unreadable file. */
     if (access (filename, R_OK) == -1)
-        return 0;
+        goto done;
     conf = virConfReadFile(filename, 0);
     if (!conf)
-        return 0;
+        goto done;
 
     p = virConfGetValue(conf, "log_with_libvirtd");
     if (p) {
@@ -125,6 +126,9 @@ int lxcLoadDriverConfig(lxc_driver_t *driver)
     }
 
     virConfFree(conf);
+
+done:
+    VIR_FREE(filename);
     return 0;
 
 no_memory:

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 97b7903a577c3ca6ef7c01de7b2ef8367392662d..c77d0990e01b81968f472da12f28f33fa0f08686 100644 (file)
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -753,6 +753,7 @@ static int lxcContainerChild( void *data )
         virReportSystemError(NULL, errno,
                              _("Failed to open tty %s"),
                              ttyPath);
+        VIR_FREE(ttyPath);
         return -1;
     }
     VIR_FREE(ttyPath);

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 0b104a1735ec54c985eb4c9854536d86cde0c307..3cecdceda719235144a7d0473b3f1ab6eb4076a8 100644 (file)
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -501,7 +501,7 @@ lxcControllerRun(virDomainDefPtr def,
 {
     int rc = -1;
     int control[2] = { -1, -1};
-    int containerPty;
+    int containerPty = -1;
     char *containerPtyPath;
     pid_t container = -1;
     virDomainFSDefPtr root;
@@ -734,7 +734,7 @@ int main(int argc, char *argv[])
         goto cleanup;
     }
 
-    if (getuid() && 0) {
+    if (getuid() != 0) {
         fprintf(stderr, "%s: must be run as the 'root' user\n", argv[0]);
         goto cleanup;
     }

diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
index fe8d1166913fe3e56e3ea130640b4ac743cef418..818c7d69d44607986b900be75df51cb935840547 100644 (file)
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -364,7 +364,7 @@ static int gather_capabilities(LibHalContext *ctx, const char *udi,
 {
     char *bus_name = NULL;
     virNodeDevCapsDefPtr caps = NULL;
-    char **hal_cap_names;
+    char **hal_cap_names = NULL;
     int rv, i;
 
     if (STREQ(udi, "/org/freedesktop/Hal/devices/computer")) {
@@ -778,11 +778,6 @@ static int halDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)
     virNodeDeviceObjListFree(&driverState->devs);
     if (hal_ctx)
         (void)libhal_ctx_free(hal_ctx);
-    if (udi) {
-        for (i = 0; i < num_devs; i++)
-            VIR_FREE(udi[i]);
-        VIR_FREE(udi);
-    }
     nodeDeviceUnlock(driverState);
     VIR_FREE(driverState);
 

diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index f4c62f33f3644736cadf7d168681351f3846d512..a7ccfdb84f2c238f758837321395ac228b4e35cb 100644 (file)
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -329,12 +329,14 @@ openvz_replace(const char* str,
                const char* to) {
     const char* offset = NULL;
     const char* str_start = str;
-    int to_len = strlen(to);
-    int from_len = strlen(from);
+    int to_len;
+    int from_len;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
 
-    if(!from)
+    if ((!from) || (!to))
         return NULL;
+    from_len = strlen(from);
+    to_len = strlen(to);
 
     while((offset = strstr(str_start, from)))
     {

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 245abb73113b782f68d6a4b5aa3f05d61db6f711..7d20a545d5005a91cb31d72f390263fbc68b3c56 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1180,9 +1180,9 @@ qemudReadLogOutput(virConnectPtr conn,
         usleep(100*1000);
         retries--;
     }
-    if (retries == 0)
-        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                         _("Timed out while reading %s log output"), what);
+
+    qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                     _("Timed out while reading %s log output"), what);
     return -1;
 }
 
@@ -4635,6 +4635,7 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
         newdisk->src = NULL;
         origdisk->type = newdisk->type;
     }
+    VIR_FREE(devname);
 
     return ret;
 }

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index c866111e9bd17ae5e2d7ea35523a961a83b75270..ba111d872ad5a7e6663284cd27bd0de440a465b3 100644 (file)
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -937,9 +937,10 @@ doRemoteOpen (virConnectPtr conn,
         if (priv->pid > 0) {
             pid_t reap;
             do {
+retry:
                 reap = waitpid(priv->pid, NULL, 0);
                 if (reap == -1 && errno == EINTR)
-                    continue;
+                    goto retry;
             } while (reap != -1 && reap != priv->pid);
         }
 #endif
@@ -1400,9 +1401,10 @@ doRemoteClose (virConnectPtr conn, struct private_data *priv)
     if (priv->pid > 0) {
         pid_t reap;
         do {
+retry:
             reap = waitpid(priv->pid, NULL, 0);
             if (reap == -1 && errno == EINTR)
-                continue;
+                goto retry;
         } while (reap != -1 && reap != priv->pid);
     }
 #endif

diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index e82959cca6ea55b331e2ab911f8dea8a65be4da7..72ccd8167a04e9a84b370fbc7aa498033089bc31 100644 (file)
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -82,12 +82,10 @@ virStorageBackendDiskMakeDataVol(virConnectPtr conn,
          * dir every time its run. Should figure out a more efficient
          * way of doing this...
          */
-        if ((vol->target.path = virStorageBackendStablePath(conn,
-                                                            pool,
-                                                            devpath)) == NULL)
-            return -1;
-
+        vol->target.path = virStorageBackendStablePath(conn, pool, devpath);
         VIR_FREE(devpath);
+        if (vol->target.path == NULL)
+            return -1;
     }
 
     if (vol->key == NULL) {
@@ -646,7 +644,7 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
 
     part_num = devname + strlen(srcname);
 
-    if (!part_num) {
+    if (*part_num == 0) {
         virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
                               _("cannot parse partition number from target "
                                 "'%s'"), devname);

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 80e4543f9919f3ab5b2a824a71d007e2e556c400..e15de28c25d5635d6580076dcbc0d8abd3ba22bf 100644 (file)
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -80,7 +80,8 @@ storageDriverAutostart(virStorageDriverStatePtr driver) {
                 backend->startPool(NULL, pool) < 0) {
                 virErrorPtr err = virGetLastError();
                 storageLog("Failed to autostart storage pool '%s': %s",
-                           pool->def->name, err ? err->message : NULL);
+                           pool->def->name, err ? err->message :
+                           "no error message found");
                 virStoragePoolObjUnlock(pool);
                 continue;
             }
@@ -90,7 +91,8 @@ storageDriverAutostart(virStorageDriverStatePtr driver) {
                 if (backend->stopPool)
                     backend->stopPool(NULL, pool);
                 storageLog("Failed to autostart storage pool '%s': %s",
-                           pool->def->name, err ? err->message : NULL);
+                           pool->def->name, err ? err->message :
+                           "no error message found");
                 virStoragePoolObjUnlock(pool);
                 continue;
             }

diff --git a/src/util/logging.c b/src/util/logging.c
index 4899de6f5df714e35f11dd44093058dc20220a69..757f78ccee568abcfd69b8b829650c94db3ac3bf 100644 (file)
--- a/src/util/logging.c
+++ b/src/util/logging.c
@@ -386,7 +386,7 @@ int virLogDefineFilter(const char *match, int priority,
     }
 
     mdup = strdup(match);
-    if (dup == NULL) {
+    if (mdup == NULL) {
         i = -1;
         goto cleanup;
     }
@@ -484,6 +484,7 @@ int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, void *data,
 
     virLogLock();
     if (VIR_REALLOC_N(virLogOutputs, virLogNbOutputs + 1)) {
+        VIR_FREE(ndup);
         goto cleanup;
     }
     ret = virLogNbOutputs++;

diff --git a/src/xen/sexpr.c b/src/xen/sexpr.c
index 81cb49f269a2f6b24cb09c585c1fdae41d86b8af..085500dc50841aaf3452172a8b950c90469f1f28 100644 (file)
--- a/src/xen/sexpr.c
+++ b/src/xen/sexpr.c
@@ -440,9 +440,6 @@ sexpr_lookup_key(const struct sexpr *sexpr, const char *node)
     for (token = strsep(&ptr, "/"); token; token = strsep(&ptr, "/")) {
         const struct sexpr *i;
 
-        if (token == NULL)
-            continue;
-
         sexpr = sexpr->u.s.cdr;
         for (i = sexpr; i->kind != SEXPR_NIL; i = i->u.s.cdr) {
             if (i->kind != SEXPR_CONS ||

diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 3c660befb8f78e92e4ab27438961dacbcd4a4947..dad07841a6148eb56a36018055201054716bad62 100644 (file)
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -1369,16 +1369,14 @@ xend_parse_sexp_desc_char(virConnectPtr conn,
                 goto no_memory;
         }
 
-        if (connectPort) {
-            if (connectHost) {
-                virBufferVSprintf(buf,
-                                  "      <source mode='connect' host='%s' service='%s'/>\n",
-                                  connectHost, connectPort);
-            } else {
-                virBufferVSprintf(buf,
-                                  "      <source mode='connect' service='%s'/>\n",
-                                  connectPort);
-            }
+        if (connectHost) {
+            virBufferVSprintf(buf,
+                              "      <source mode='connect' host='%s' service='%s'/>\n",
+                              connectHost, connectPort);
+        } else {
+            virBufferVSprintf(buf,
+                              "      <source mode='connect' service='%s'/>\n",
+                              connectPort);
         }
         if (bindPort) {
             if (bindHost) {

diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
index 5878ba15537baccd9705462094b7c1b8c0dd18f7..f833ce7efeaf8ee257cd1c5a09ac75e836dd10a9 100644 (file)
--- a/src/xen/xm_internal.c
+++ b/src/xen/xm_internal.c
@@ -576,8 +576,7 @@ int xenXMConfigCacheRefresh (virConnectPtr conn) {
     virHashRemoveSet(priv->configCache, xenXMConfigReaper, xenXMConfigFree, &args);
     ret = 0;
 
-    if (dh)
-        closedir(dh);
+    closedir(dh);
 
     return (ret);
 }
@@ -2229,8 +2228,9 @@ virConfPtr xenXMDomainConfigFormat(virConnectPtr conn,
     if (xenXMConfigSetInt(conf, "vcpus", def->vcpus) < 0)
         goto no_memory;
 
-    if (def->cpumask &&
-        !(cpus = virDomainCpuSetFormat(conn, def->cpumask, def->cpumasklen)) < 0)
+    if ((def->cpumask != NULL) &&
+        ((cpus = virDomainCpuSetFormat(conn, def->cpumask,
+                                       def->cpumasklen)) == NULL))
         goto cleanup;
 
     if (cpus &&