6 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/