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

[Wenchao Xia]

于 2012-10-24 20:58, 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.
>
   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