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

On 10/27/2011 05:59 PM, Daniel P. Berrange wrote: > 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. Hmm, I just saw codes again, it indeed is a issue, however, I fixed a error place :-( 882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i < nmounts ; i++) { 887 VIR_DEBUG("Umount %s", mounts[i]); *Notes: dereference a NULL pointer in here.*