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

[libvirt] Re: libvirt code review



On Tue, Nov 10, 2009 at 01:00:36PM +0100, Daniel Veillard wrote:

> commit 680ced4bde478006a2e6445801993e110a3687be
> Author: Daniel Veillard <veillard 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 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 &&

ACK

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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