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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
commit 680ced4bde478006a2e6445801993e110a3687be
Author: Daniel Veillard <veillard@xxxxxxxxxx>
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@xxxxxxxxxx> 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 &&
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list