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

[Wenchao Xia]

> On Mon, Oct 22, 2012 at 5:38 AM, Wenchao Xia <xiawenc at linux.vnet.ibm.com> wrote:
>>    Oringinal implement have risk, this patch should fix it
>>
>> Signed-off-by: Wenchao Xia <xiawenc at 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