[libvirt] Re: libvirt code review
Daniel P. Berrange
berrange at redhat.com
Tue Nov 10 16:43:02 UTC 2009
On Tue, Nov 10, 2009 at 01:00:36PM +0100, Daniel Veillard wrote:
> 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 &&
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 :|
More information about the libvir-list
mailing list