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

HE> + if (!provider_is_responsible(broker, ref, &s)) HE> + goto out; HE> + HE> + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); HE> + if (conn == NULL) { HE> + cu_statusf(broker, &s, HE> + CMPI_RC_ERR_FAILED, HE> + "Could not connect to hypervisor"); HE> + goto out; HE> + } HE> inst_list_init(&device_pool_list); HE> inst_list_init(&alloc_cap_list); HE> - HE> - if (!provider_is_responsible(broker, ref, &s)) HE> - goto out; HE> - HE> - conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); HE> - if (conn == NULL) { HE> - cu_statusf(broker, &s, HE> - CMPI_RC_ERR_FAILED, HE> - "Could not connect to hypervisor"); HE> - goto out; HE> - } Moving these functions above the inst_list_init() calls will cause a crash on error, because the out target tries to free them. 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? 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. 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. 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. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms(a)us.ibm.com