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

[Heidi Eckhart]

Dan Smith wrote:
> Moving these functions above the inst_list_init() calls will cause a
> crash on error, because the out target tries to free them.
>   
Mhh, as we have many free operations now that can handle NULL pointers, 
I suggest to also make inst_list_free() able to handle NULL pointers.
> As far as I can tell, these aren't related to the rest of the patch in
> any way.  If there is something to fix here, perhaps we can call it
> out and make it a distinct patch?
>   
Agreed. I will cook up a patch for inst_list_free(). The intention to 
move the inst_list_init() functions below connect_by_classname() was to 
avoid some cycles, because the case where the provider exits right after 
provider_is_responsible() or connect_by_classname() can happen often.
> HE> @@ -138,19 +138,25 @@ static CMPIStatus alloc_cap_instances(co
> HE>          if (id && !inst_id) {
> HE>              cu_statusf(broker, &s,
> HE>                         CMPI_RC_ERR_NOT_FOUND,
> HE> -                       "Requested Object could not be found.");
> HE> +                       "No such instance (%s)", id);
>
> Also, this change doesn't seem to be necessary, and makes this one
> error message inconsistent with many others that we already have.  If
> this is a personal preference change, then it should be a distinct
> patch that either changes (or plans to change) the other instances of
> these error messages.
>   
Agreed. I saw that we already have a task for "Fix up all the error 
codes in returned status values (everything is currently 
CMPI_RC_ERR_FAILED)". So we should leave this discussion for there. I 
will remove this change.
> HE> -        if (names_only)
> HE> -                cu_return_instance_names(results, &alloc_cap_list);
> HE> -        else
> HE> -                cu_return_instances(results, &alloc_cap_list);
> HE> +        if (results) {
> HE> +                if (names_only)
> HE> +                        cu_return_instance_names(results, &alloc_cap_list);
> HE> +                else
> HE> +                        cu_return_instances(results, &alloc_cap_list);
> HE> +        } else {
> HE> +                if (inst)
> HE> +                        *inst = alloc_cap_list.list[0];
> HE> +        }
>
> I'm not okay with this change.  From what I can see, this function was
> modified to have a dual return path, where either results is passed
> in, and inst is NULL, or vice versa.  If you want to have the
> opportunity to intercept the instances coming back, then the function
> should take in an inst_list, and populate it.  The caller can then
> either return or inspect the result.
>   
Very good catch. Sometimes the goal to make the best out of the existing 
code makes oneself blind for bigger changes. Thanks.
> Other places in the code, we have a function as described, and then a
> pass-through function called "return_foo()" that calls the function
> that returns the list, and then calls cm_return_instance...().  Such a
> function seems appropriate here.
>   
>


-- 
Regards

Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor