[libvirt] [PATCH] lxc: remove redundant check

[Alex Jia]

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.
Yeah, you're right, thanks a lot.

Alex
> Regards,
> Daniel