[libvirt] [PATCH] lxc: remove redundant check

[Alex Jia]

On 10/27/2011 06:46 PM, Michal Privoznik wrote:
> On 27.10.2011 12:39, Alex Jia wrote:
>> 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 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.
>> 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.*
> The only way for nmounts being nonzero is mounts being non NULL;
Indeed, Michal, thanks for your comment :)

Alex
> Thus, if mounts is NULL, there is no way for nmounts being something
> else than 0; Hence, the body of this for will not be executed.
>
> I think the code is good as-is.
>
> Michal