[libvirt] [PATCH] lxc: remove redundant check

[Michal Privoznik]

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;
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