[libvirt] Re: libvirt code review

Daniel Veillard veillard at redhat.com
Tue Nov 10 12:00:36 UTC 2009


On Thu, Nov 05, 2009 at 01:12:41PM -0500, Steve Grubb wrote:
> Hello,
> 
> The following is against 0.7.2-3 from F-13 cvs. I mention each source file at 
> the beginning of a block and have a blank line indicating that we are moving 
> along to the next file. Hope you find this helpful.
> 
> -Steve

  Okay I fixed the issues on a 0.7.2 branch and then backported the
patch back onto git head, except for a few exceptions, everything was
still applying !

> In src/util/storage_file.c at line 192, there is a test that I presume is for 
> -1. The problem is that its a variable that is an unsigned long is 64 bits on 

    unsigned long size;

    size = ((buf[4+4+8] << 24)
            | (buf[4+4+8+1] << 16)
            | (buf[4+4+8+2] << 8)
            | buf[4+4+8+3]); /* QCowHeader.backing_file_size */

[...]

    if (size + 1 == 0)
        return BACKING_STORE_INVALID;
    if (VIR_ALLOC_N(*res, size + 1) < 0) {

  I think it's just to make 100% sure the size + 1 computation on the
following line won't overflow. So current code looks fine to me

> some arches. It would probably be better to use a unit32_t for size or change 
> the test. The bottom line is the test will evaluate false on some arches.

  unit32_t is nowhere used in libvirt code I would rather not use it,
we could use size_t there since the goal is to allocate memory but I'm
not 100% sure it's right, basically the test make sure we won't try to
allocate more than 4G of memory in one chunk.

> In src/datatypes.c at lines 335, 476, and 790, there is a test to see if uuid 
> is NULL. There was a test in the beginning of the function and it returned if 
> this was true, so this "if" statement can be deleted in all 3 cases.

  Hum, this is the virGetxxx() functions, they lookup objects given the
name or uuid, and hold big TODOs comments at the start:

   /* TODO search by UUID first as they are better differenciators */
   /* TODO check the UUID */

  And somehow I think the intend was to allow lookup only by name, with
the uuid being unknown, I would think the proper fix is actually to drop
the || (uuid == NULL) at the start of the function.

> In src/libvirt.c at line 1122, there is an "if" statement that "ands" 0 with 
> the return from STREQ. Should that have been a == ? The test evaluates false 
> as is.

   Comparing with others similar message being emitted in other places,
I think '0 && ' is just a bogus remain, I'm removing it.

> At line 3214, there is a test if uri == NULL. Then within that "if" 
> statement at line 3216 is a test for if uri is still NULL. Should that have 
> been a test for dstURI being NULL?

  yup ! That had been fixed in head since though.

> In src/remote/remote_driver.c at line 939 is a do loop. By design, it seems 
> the intention was to reloop if waitpid returns -1 and EINTR is set. The 
> implementation doesn't match it. If -1 is returned, it will do the continue, 
> which is to jump to the end of the do loop and it evaluates reap which would 
> be a -1 and it will exit. I suspect instead of continue, it should "goto 
> again" and again would be placed before the call to waitpid.
> This problem is found again at line 1406

  okay, nasty, somehow it's easy to think continue will retry from top of
the loop not including the test

> In src/xen/sexpr.c at line 443 is a test for token == NULL. Token is 
> guaranteed to not be NULL because of the loop entry test at line 440.

  okay

> In src/xen/xend_internal.c at line 1367 is a test for connectPort being true. 
> Prior to this, connectPort is guranteed to be valid or it would have jumped to 
> no_memory.

  okay as the affectation is done in both if and else parts of the
preceeding test.

> In src/xen/xm_internal.c at line 579 is a test for dh being valid. It is 
> guaranteed to be valid since a -1 was returned from the function on a failed 
> call to opendir.

  okay

> At line 2219 is an unusual if statement. Normally you do not see something 
> constructed as (!cpu)<0). That would seem to have meant cpu>=0 which is more 
> straightforward.

    if (def->cpumask &&
        !(cpus = virDomainCpuSetFormat(conn, def->cpumask,
def->cpumasklen)) < 0)       
        goto cleanup;

    But the function returns a string or NULL in case of error, I try to
fight again not fully parenthetized test expressions but I'm loosing
that fight unfortunately :-(

   in any case the < 0 test is bogus since it's a string

    if ((def->cpumask) &&
        ((cpus = virDomainCpuSetFormat(conn, def->cpumask,
         def->cpumasklen)) == NULL))
        goto cleanup

seems the right construct to me, as it tests if the call failed.

> In src/phyp/phyp_driver.c at line 1008, there is a strdup inside an if 
> statement with nothing catching its returned memory. Two lines later it does 
> the same thing again but puts it in an array. Not sure what the if statement 
> should have been checking, but its definitely a memory leak as is.
> At line 1040 is an allocation. If that fails, it passes dom->comm to an error 
> reporting function. However, at line 1034, dom was set to NULL and is not 
> changed.
> The same problem appears again at line 1085.
> At line 1105 is a test for exit_status < 0. Nothing has changed it since it 
> was initialized.

  That code was completely revamped since, none of this applies now, it
was pretty bad ! C.f. https://bugzilla.redhat.com/show_bug.cgi?id=529977

> In src/openvz/openvz_conf at line 336 is a test for from being NULL. However, 
> it was previously used at line 333 before checking if it was NULL.

  yeah, there was another passed pointer which should have got the same
check too, cleaning this up.

> In src/qemu/qemu_monitor_text.c at line 178, a variable, control, is assigned 
> to a member of the msg structure. However, control goes out of scope before 
> its used. It should be in the function level declarations.

  Ah right, nasty !

> In src/qemu/qemu_driver.c at line 894 is a test for retries being 0. This will 
> always be true since the loop has no break statements.
> At line 4484, we leave the function without freeing devname.

  okay,

> In src/lxc/lxc_conf.c at line 111, 114, and 125, we leave the function without 
> freeing filename.

  okay

> In src/lxc/lxc_container.c at line 743, we leave the function without freeing 
> ttyPath.

  okay

> In src/storage/storage_driver.c at lines 83 and 93 we call storageLog/fprintf 
> with the second parameter to %s being NULL. Wasn't an empty string intended?

  I used "no error message found" as we failed to gather an error message

> In src/storage/storage_backend_disk.c at line 88, we leave the function 
> without freeing devpath.

  okay

> At line 649 is a test for part_num being NULL. Can it ever be NULL?

  No, but we should check for an empty string, done

> In src/node_device/node_device_hal.c at line 405 is a test for hal_cap_names 
> being true, its conceivable to get there without it being initialized. For 
> example, line 373 could jump to it. It should probably be initialized to 0.

  okay

> At line 781 is a test for udi being true. It will alwys be false since once 
> its defined it will never jump to failure.
> At line 782 is a use of num_devs without it being initialized. If the problem 
> mentioned for udi is fixed by deleting all this code, then the problem goes 
> away.

  okay removing that block, I just hope we won't add a failure jump from
within the loop

> In src/lxc/lxc_controller.c at line 633 is a test for containerPty not being
> -1. Its conceivable to get here with it notr being initialized perhaps at line
> 512 for example. It should probably be initialized to a -1.

  yup, okay

> At line 735 is an if statement "anding" 0 with getuid(). this should probably
> be a != rather than a &&.

  right since the error message clearly indicate we expect this to run
as root.

  Thanks a lot for the review !
Patch for git head attached

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/
-------------- next part --------------
commit 680ced4bde478006a2e6445801993e110a3687be
Author: Daniel Veillard <veillard at redhat.com>
Date:   Tue Nov 10 12:56:11 2009 +0100

    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 at 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 2c50790..9e80e29 100644
--- 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 74dc367..bc81543 100644
--- 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 97b7903..c77d099 100644
--- 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 0b104a1..3cecdce 100644
--- 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 fe8d116..818c7d6 100644
--- 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 6eeece8..a50e4d6 100644
--- 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 f022f89..08a9b42 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -886,9 +886,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;
 }
 
@@ -4352,6 +4352,7 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
         newdisk->src = NULL;
         origdisk->type = newdisk->type;
     }
+    VIR_FREE(devname);
 
     return ret;
 }
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 35cd330..ab1fb9a 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -159,6 +159,7 @@ qemuMonitorSendUnix(const virDomainObjPtr vm,
                     size_t cmdlen,
                     int scm_fd)
 {
+    char control[CMSG_SPACE(sizeof(int))];
     struct msghdr msg;
     struct iovec iov[1];
     ssize_t ret;
@@ -172,7 +173,6 @@ qemuMonitorSendUnix(const virDomainObjPtr vm,
     msg.msg_iovlen = 1;
 
     if (scm_fd != -1) {
-        char control[CMSG_SPACE(sizeof(int))];
         struct cmsghdr *cmsg;
 
         msg.msg_control = control;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index c866111..ba111d8 100644
--- 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 e82959c..72ccd81 100644
--- 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 80e4543..e15de28 100644
--- 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 4899de6..757f78c 100644
--- 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 81cb49f..085500d 100644
--- 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 3c660be..dad0784 100644
--- 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 5878ba1..f833ce7 100644
--- 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 &&


More information about the libvir-list mailing list