Fwd: Re: [PATCH 4/4] DevicePool, reimplement get_diskpool_config with libvirt
On Mon, Oct 22, 2012 at 5:38 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
Oringinal implement have risk, this patch should fix it
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- 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). -- Best Regards Wenchao Xia
On Wed, Oct 24, 2012 at 3:52 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
On Mon, Oct 22, 2012 at 5:38 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
Oringinal implement have risk, this patch should fix it
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- 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.
No, if count is 0 the for will never be executed, this is basic C. There is no need to test count before starting the loop. See test attached. As for coding style, when you know the number of iterations, the for loop it is much more readable and easier to maintain.
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).
Yes, I reported this a long time ago, the tests _only_ fail if you have SELinux enabled. Try disabling it and running again to check the results. Cheers, -- Eduardo de Barros Lima ◤✠◢ eblima@gmail.com
于 2012-10-24 20:58, Eduardo Lima (Etrunko) 写道:
On Wed, Oct 24, 2012 at 3:52 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
On Mon, Oct 22, 2012 at 5:38 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
Oringinal implement have risk, this patch should fix it
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- 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.
No, if count is 0 the for will never be executed, this is basic C. There is no need to test count before starting the loop. See test attached. As for coding style, when you know the number of iterations, the for loop it is much more readable and easier to maintain.
My mistake, I must have read it from some mis guiding books, for will look better.
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).
Yes, I reported this a long time ago, the tests _only_ fail if you have SELinux enabled. Try disabling it and running again to check the results.
Perhaps SeLinux is another trigger of the cimtest error, Just found that there is some problem if libvirt-cim call libvirt event API, if I comment it out, test pass, otherwise fail, so will send a patch bring back libvirt-cim CSI, and leave libvirt event CSI as 2nd option.
Cheers,
-- Best Regards Wenchao Xia
participants (2)
-
Eduardo Lima (Etrunko) -
Wenchao Xia