Re: [libvirt] [PATCH] lxc: remove redundant check

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(a)redhat.com wrote: >>>>> From: Alex Jia&lt;ajia(a)redhat.com&gt; >>>>> >>>>> 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&lt;ajia(a)redhat.com&gt; >>>>> --- >>>>> 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