[Libvirt-cim] [PATCH] Fix AllocationCapabilities to make EnumInstances and EnumInstanceNames work

[Jay Gagnon]

Dan Smith wrote:
> JG> +        for (i = 0; device_pool_names[i]; i++) {
> JG> +                s = get_pool_by_type(broker,
> JG> +                                     conn,
> JG> +                                     device_pool_names[i],
> JG> +                                     NAMESPACE(ref),
> JG> +                                     &device_pool_list);
> JG> +                if (s.rc != CMPI_RC_OK) {
> JG> +                        cu_statusf(broker, &s,
> JG> +                                   CMPI_RC_ERR_FAILED,
> JG> +                                   "Error fetching device pools");
> JG> +                        goto out;
> JG> +                }
> JG> +        }
>
> We do this elsewhere too.  Perhaps we need a get_all_pools() function
> in Virt_DevicePool?
>   
Sounds reasonable.  I'll do that.
> JG> +
> JG> +        for (i = 0; i < device_pool_list.cur; i++) {
> JG> +                inst = get_typed_instance(broker,
> JG> +                                          "AllocationCapabilities",
> JG> +                                          NAMESPACE(ref));
> JG> +                if (inst == NULL) {
> JG> +                        cu_statusf(broker, &s,
> JG> +                                   CMPI_RC_ERR_FAILED,
> JG> +                                   "Could not get alloc_cap instance");
> JG> +                        goto out;
> JG> +                }
> JG> +
> JG> +                s = cu_copy_prop(broker, device_pool_list.list[i], inst, 
> JG> +                                 "InstanceID", NULL);
>
> This function looks great, but I think we're missing the patch that
> adds it to libcmpiutil :)
>   
Oops.  I'll send that out when I send the next iteration of this.
> JG> +                if (s.rc != CMPI_RC_OK) {
> JG> +                        cu_statusf(broker, &s,
> JG> +                                   CMPI_RC_ERR_FAILED,
> JG> +                                   "Error copying InstanceID");
> JG> +                        goto out;
> JG> +                }
> JG> +                
> JG> +                s = cu_copy_prop(broker, device_pool_list.list[i], inst, 
> JG> +                                 "ResourceType", NULL);
> JG> +                if (s.rc != CMPI_RC_OK) {
> JG> +                        cu_statusf(broker, &s,
> JG> +                                   CMPI_RC_ERR_FAILED,
> JG> +                                   "Error copying InstanceID");
> JG> +                        goto out;
> JG> +                }
> JG> +                
> JG> +                inst_list_add(&alloc_cap_list, inst);
> JG> +        }
>
> To reduce the size of this loop, and to be in line with our other
> instance providers, I think it would be good to define a function that
> looks something like this:
>
>   CMPIStatus get_ac_instance(CMPIInstance *pool, CMPIInstance **ac);
>
> This would encapsulate things a bit and also would provide a clean
> hook for ElementCapabilities, which will need to, given a pool, get an
> instance of AllocationCapabilities.
>   
Sure, no problem.  Given all the inexplicable struggle getting that bit
working I guess I didn't want to look at it anymore, so the lack of
encapsulation didn't really strike me.  Easy fix though, and good call.

-- 

-Jay