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