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<ajia(a)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(a)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