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

[Eduardo Lima (Etrunko)]

On Wed, Oct 24, 2012 at 3:52 AM, Wenchao Xia <xiawenc at linux.vnet.ibm.com> wrote:
>
>> 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.

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 at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_for.c
Type: text/x-csrc
Size: 158 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvirt-cim/attachments/20121024/6392edf2/attachment.bin>