Re: [Libvirt-cim] [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC
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