Re: [Libvirt-cim] [PATCH 4/4] DevicePool, reimplement get_diskpool_config with libvirt

> On Mon, Oct 22, 2012 at 5:38 AM, Wenchao Xia <xiawenc(a)linux.vnet.ibm.com&gt; > wrote: >> >> Oringinal implement have risk, this patch should fix it >> >> Signed-off-by: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com&gt; >> --- >> src/Virt_DevicePool.c | 47 >> +++++++++++++++++++++++++++++++++++------------ >> 1 files changed, 35 insertions(+), 12 deletions(-) >> >> diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c >> index 79dc108..0cb9124 100644 >> --- a/src/Virt_DevicePool.c >> +++ b/src/Virt_DevicePool.c >> @@ -117,52 +117,75 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct >> virt_pool **pool) >> return ret; >> } >> >> +/* This function returns the real number of pools, no negative value >> should be >> + returned, if error happens it returns zero. */ >> static int get_diskpool_config(virConnectPtr conn, >> struct tmp_disk_pool **_pools) >> { >> - int count = 0; >> + int count = 0, realcount = 0; >> int i; >> char ** names = NULL; >> struct tmp_disk_pool *pools = NULL; >> + int have_err = 0; >> >> count = virConnectNumOfStoragePools(conn); >> - if (count <= 0) >> + if (count <= 0) { >> + have_err = 1; >> goto out; >> + } >> >> names = calloc(count, sizeof(char *)); >> if (names == NULL) { >> CU_DEBUG("Failed to alloc space for %i pool names", >> count); >> count = 0; >> + have_err = 1; >> goto out; >> } >> >> - if (virConnectListStoragePools(conn, names, count) == -1) { >> + realcount = virConnectListStoragePools(conn, names, count); >> + if (realcount == -1) { >> CU_DEBUG("Failed to get storage pools"); >> - count = 0; >> + realcount = 0; >> + have_err = 1; >> + goto out; >> + } >> + if (realcount == 0) { >> + CU_DEBUG("zero pools got, but prelist is %d.", count); >> goto out; >> } >> >> - pools = calloc(count, sizeof(*pools)); >> + pools = calloc(realcount, sizeof(*pools)); >> if (pools == NULL) { >> - CU_DEBUG("Failed to alloc space for %i pool structs", >> count); >> + CU_DEBUG("Failed to alloc space for %i pool structs", >> realcount); >> + realcount = 0; >> + have_err = 1; >> goto out; >> } >> >> - for (i = 0; i < count; i++) { >> + i = 0; >> + while (i < realcount) { >> pools[i].tag = strdup(names[i]); >> pools[i].primordial = false; >> + i++; >> } > > > Any specific reason for changing the for() loop for a while() one?? > >> >> out: >> - for (i = 0; i < count; i++) >> - free(names[i]); >> - free(names); >> + if (count > 0) { >> + i = 0; >> + while (i < count) { >> + free(names[i]); >> + i++; >> + } >> + free(names); >> + } > > > Same here. > > Best regards, > Good to see you again, there is one for() before which may take one execution if count == 0,where it should not. For safe and code style unifying I switch it all to while.

I am tring to fix some bugs after libvirt CSI patch applied, which make cimtest report strange error, A bit brute, could u help share some findings for those strange errors? (2 profile test case failing strangely, if CSI test case is executed with CSI patched libvirt-cim).