Re: [Libvirt-cim] [PATCH] [CU] Merge do_ref() and do_assoc()

HE> # HG changeset patch HE> # User Heidi Eckhart <heidieck(a)linux.vnet.ibm.com&gt; HE> # Date 1197367712 -3600 HE> # Node ID 8ef14daa017090e8473e1f958bc5e5e584032398 HE> # Parent b5847180d00719ff39f2d451d4e79d4369952ad5 HE> [CU] Merge do_ref() and do_assoc() I'm supportive of this change, however, I have a few comments. HE> + /* Associators and AssociatorNames */ HE> + if (!ref_rslt) { HE> + s = filter_results(&list, HE> + NAMESPACE(ref), HE> + info->result_class, HE> + ctx->brkr); HE> + if (s.rc != CMPI_RC_OK) { HE> + CU_DEBUG("filter_results did not return CMPI_RC_OK."); HE> + goto out; HE> + } HE> + HE> + if (list.list == NULL) { HE> + CU_DEBUG("List is empty."); HE> + goto out; HE> + } else { HE> + CU_DEBUG("Returned %u instance(s).", list.cur); HE> + } HE> + } Adding these additional clauses to do_assoc() makes the function too long and confusing, IMHO. It was already rather long after recent additions. Lets take this opportunity to refine it a bit. I think that we should be able to boil down the (ref_rslt == NULL) and (ref_rslt != NULL) cases into two small helper functions. Each should build a result list. Thus, we can have two lists, one that is the result of the handler, and one that is to be returned. So, we can clean it up like this: if (ref_rslt) ref_case(&handler_result, &to_return, info); else assoc_case(&handler_result, &to_return, info); if (names_only) cu_return_instance_names(results, &to_return); else cu_return_instances(results, &to_return); The filtering case of associators would be in assoc_case(), the make_ref() calls would be in ref_case(). HE> + /* References and ReferenceNames */ HE> + if (ref_rslt) { HE> + for (i = 0; i < list.cur; i++) { HE> + CMPIInstance *refinst; HE> + HE> + refinst = handler->make_ref(ref, list.list[i], info, handler); HE> + if (refinst == NULL) HE> + continue; HE> + HE> + if (names_only) HE> + cu_return_instance_name(results, refinst); HE> + else HE> + CMReturnInstance(results, refinst); HE> + } HE> + } HE> + /* Associators and AssociatorNames */ HE> + else { HE> if (names_only) HE> - cu_return_instance_name(results, refinst); HE> + cu_return_instance_names(results, &list); HE> else HE> - CMReturnInstance(results, refinst); HE> - } HE> - HE> - CMSetStatus(&s, CMPI_RC_OK); HE> + cu_return_instances(results, &list); HE> + } HE> + This would then clear up this (IMHO, very confusing) case where you first have a associators case, then an "if references, else associators" case right behind it. Thoughts? If I have been too confusing in my explanation, I will be glad to take your patch and post a modified version to illustrate my intent. Thanks Heidi! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms(a)us.ibm.com