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 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@redhat.com wrote: >> From: Alex Jia<ajia@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@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
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: 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