[libvirt] Re: libvirt code review

commit 680ced4bde478006a2e6445801993e110a3687be Author: Daniel Veillard <veillard(a)redhat.com&gt; 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(a)redhat.com&gt; 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 &&