[libvirt] [PATCH] lxc: remove redundant check

[Daniel P. Berrange]

On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote:
> On 10/27/2011 05:26 PM, Daniel P. Berrange wrote:
> >On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
> >>On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
> >>>On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia at redhat.com wrote:
> >>>>From: Alex Jia<ajia at redhat.com>
> >>>>
> >>>>Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1)
> >>>>can make sure mounts is a none null value, so it is redundant to check if
> >>>>mounts is null.
> >>>VIR_REALLOC_N is only executed inside the loop. Although it is unlikely,
> >>>in theory there can be zero iterations of the loop, in which case
> >>>'mounts' will be NULL at the point qsort is called.
> >>>
> >>>>* src/lxc/lxc_container.c: remove redundant check.
> >>>>
> >>>>Signed-off-by: Alex Jia<ajia at redhat.com>
> >>>>---
> >>>>  src/lxc/lxc_container.c |    5 ++---
> >>>>  1 files changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>>diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> >>>>index 06ccf7e..f4879c3 100644
> >>>>--- a/src/lxc/lxc_container.c
> >>>>+++ b/src/lxc/lxc_container.c
> >>>>@@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void)
> >>>>      }
> >>>>      endmntent(procmnt);
> >>>>
> >>>>-    if (mounts)
> >>>>-        qsort(mounts, nmounts, sizeof(mounts[0]),
> >>>>-              lxcContainerChildMountSort);
> >>>>+    qsort(mounts, nmounts, sizeof(mounts[0]),
> >>>>+          lxcContainerChildMountSort);
> >>>>
> >>>>      for (i = 0 ; i<   nmounts ; i++) {
> >>>>          VIR_DEBUG("Umount %s", mounts[i]);
> >>>NACK, the check for NULL is correct
> >>>
> >>>
> >>>Daniel
> >>Hi Daniel,
> >>My comment is wrong, but the following judgement should make sure
> >>mounts is non null:
> >><snip>
> >>         if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) {
> If the previous VIR_REALLOC_N can't make sure 'mounts' is non null,
> whether we need to check 'mounts' to avoid dereferencing a NULL
> pointer in here again, for instance:
> 
> if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) {

The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there
is at least 1 iteration of the while() loop.  The "if (mounts) qsort"
code is *outside* the while() loop, and has to cope with the scenario
where the while() loop had *zero* iterations.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|