[Libvirt-cim] [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC

[Heidi Eckhart]

Dan Smith wrote:
> HE> +        CMPIStatus s = {CMPI_RC_OK, NULL};
> HE> +        CMPIInstance *alloc_cap_inst;
> HE> +        virConnectPtr conn = NULL;
> HE> +        struct inst_list device_pool_list;
> HE> +        const char *inst_id;
> HE>          int i;
> HE> -        virConnectPtr conn = NULL;
> HE> -        CMPIInstance *alloc_cap_inst;
> HE> -        struct inst_list alloc_cap_list;
> HE> -        struct inst_list device_pool_list;
> HE> -        CMPIStatus s = {CMPI_RC_OK, NULL};
> HE> -        const char *inst_id;
>
> Reordering this block for no reason makes it really difficult to tell
> what is changed.  Can we please keep this kind of thing separate from
> the functional change to the code?
>   
Sorry, this happened as changes went forward. I will fix this.
> HE> +        inst_list_init(list);
>
> In other places, we initialize the list before we pass it into a
> function.  I think it makes more sense that way, as the caller may
> want the results of this function appended to the list it passes in.
>
> HE> +static CMPIStatus return_alloc_cap_instances(const CMPIBroker *broker,
> HE> +                                             const CMPIObjectPath *ref,
> HE> +                                             const CMPIResult *results,
> HE> +                                             bool names_only,
> HE> +                                             const char **properties,
> HE> +                                             const char *id)
> HE> +{
> HE> +        CMPIStatus s = {CMPI_RC_OK, NULL};
> HE> +        struct inst_list *list = NULL;
> HE> +
> HE> +        s = enum_alloc_cap_instances(broker,
> HE> +                                     ref,
> HE> +                                     properties,
> HE> +                                     id,
> HE> +                                     list);
> HE> +        if (s.rc != CMPI_RC_OK)
> HE> +                goto out;
> HE> +        
> HE> +        if (names_only)
> HE> +                cu_return_instance_names(results, list);
> HE> +        else
> HE> +                cu_return_instances(results, list);
>
> This doesn't work, does it?  You pass a NULL list pointer in to the
> function, it allocates a list, but has no way to return it back.
> You're passing just a single pointer by value here.  If you were
> modifying what the pointer pointed to, then you could get the result,
> but otherwise the list that the enum_alloc_cap_instances() function
> generates isn't visible to this caller.
>
> You should do the following:
>
>   struct inst_list list;
>
>   inst_list_init(&list);
>
>   s = enum_alloc_cap_instances(broker,
>                                ref,
>                                properties,
>                                id,
>                                &list);
>
> Which addresses this issue as well as the previous one.
>
> Thanks!
>
>   
Oh, oh, what a bad day in my development time. You are absolutely right. 
This setup has no chance to return the result to the caller. And I 
haven't tested as extensively as I should have done.
An excellent catch. Thank you. :)


-- 
Regards

Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor