[PATCH] [CU] Merge do_ref() and do_assoc()
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1197367712 -3600 # Node ID 8ef14daa017090e8473e1f958bc5e5e584032398 # Parent b5847180d00719ff39f2d451d4e79d4369952ad5 [CU] Merge do_ref() and do_assoc() The two functions do_ref() and do_assoc() run through the same logic. The only difference are the returned instances. By configuring the do_assoc() function with an additional paramter (ref_rslt) to inidcate the type of the returned instance(s) - either reference(names) or associator(names) - the two functions can be merged. Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r b5847180d007 -r 8ef14daa0170 std_association.c --- a/std_association.c Fri Dec 07 17:15:07 2007 -0500 +++ b/std_association.c Tue Dec 11 11:08:32 2007 +0100 @@ -226,21 +226,23 @@ static CMPIStatus do_assoc(struct std_as struct std_assoc_info *info, const CMPIResult *results, const CMPIObjectPath *ref, + bool ref_rslt, bool names_only) { CMPIStatus s = {CMPI_RC_OK, NULL}; struct inst_list list; struct std_assoc *handler; - - inst_list_init(&list); + int i; CU_DEBUG("Getting handler..."); handler = std_assoc_get_handler(ctx, info, ref); if (handler == NULL) { CU_DEBUG("No handler found."); - goto out; + return s; } CU_DEBUG("Getting handler succeeded."); + + inst_list_init(&list); CU_DEBUG("Calling handler->handler..."); s = handler->handler(ref, info, &list); @@ -257,73 +259,48 @@ static CMPIStatus do_assoc(struct std_as goto out; } - s = filter_results(&list, - NAMESPACE(ref), - info->result_class, - ctx->brkr); - if (s.rc != CMPI_RC_OK) { - CU_DEBUG("filter_results did not return CMPI_RC_OK."); - goto out; - } - - if (list.list == NULL) { - CU_DEBUG("List is empty."); - goto out; - } else { - CU_DEBUG("Returned %u instance(s).", list.cur); - } - - if (names_only) - cu_return_instance_names(results, &list); - else - cu_return_instances(results, &list); - - out: - inst_list_free(&list); - - return s; -} - -static CMPIStatus do_ref(struct std_assoc_ctx *ctx, - struct std_assoc_info *info, - const CMPIResult *results, - const CMPIObjectPath *ref, - bool names_only) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - struct inst_list list; - struct std_assoc *handler; - int i; - - inst_list_init(&list); - - CU_DEBUG("Getting handler..."); - handler = std_assoc_get_handler(ctx, info, ref); - if (handler == NULL) { - CU_DEBUG("No handler found."); - goto out; - } - CU_DEBUG("Getting handler succeeded."); - - CU_DEBUG("Calling handler->handler..."); - s = handler->handler(ref, info, &list); - if ((s.rc != CMPI_RC_OK) || (list.list == NULL)) - goto out; - - for (i = 0; i < list.cur; i++) { - CMPIInstance *refinst; - - refinst = handler->make_ref(ref, list.list[i], info, handler); - if (refinst == NULL) - continue; - + /* Associators and AssociatorNames */ + if (!ref_rslt) { + s = filter_results(&list, + NAMESPACE(ref), + info->result_class, + ctx->brkr); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("filter_results did not return CMPI_RC_OK."); + goto out; + } + + if (list.list == NULL) { + CU_DEBUG("List is empty."); + goto out; + } else { + CU_DEBUG("Returned %u instance(s).", list.cur); + } + } + + /* References and ReferenceNames */ + if (ref_rslt) { + for (i = 0; i < list.cur; i++) { + CMPIInstance *refinst; + + refinst = handler->make_ref(ref, list.list[i], info, handler); + if (refinst == NULL) + continue; + + if (names_only) + cu_return_instance_name(results, refinst); + else + CMReturnInstance(results, refinst); + } + } + /* Associators and AssociatorNames */ + else { if (names_only) - cu_return_instance_name(results, refinst); + cu_return_instance_names(results, &list); else - CMReturnInstance(results, refinst); - } - - CMSetStatus(&s, CMPI_RC_OK); + cu_return_instances(results, &list); + } + out: inst_list_free(&list); @@ -359,6 +336,7 @@ CMPIStatus stda_AssociatorNames(CMPIAsso &info, results, reference, + false, true); } @@ -386,6 +364,7 @@ CMPIStatus stda_Associators(CMPIAssociat &info, results, reference, + false, false); } @@ -406,7 +385,12 @@ CMPIStatus stda_ReferenceNames(CMPIAssoc self->ft->miName, }; - return do_ref(self->hdl, &info, results, reference, true); + return do_assoc(self->hdl, + &info, + results, + reference, + true, + true); } CMPIStatus stda_References(CMPIAssociationMI *self, @@ -427,7 +411,12 @@ CMPIStatus stda_References(CMPIAssociati self->ft->miName, }; - return do_ref(self->hdl, &info, results, reference, false); + return do_assoc(self->hdl, + &info, + results, + reference, + true, + false); } /*
Heidi Eckhart wrote:
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1197367712 -3600 # Node ID 8ef14daa017090e8473e1f958bc5e5e584032398 # Parent b5847180d00719ff39f2d451d4e79d4369952ad5 [CU] Merge do_ref() and do_assoc()
Nice change Heidi! I like how this condenses things. I did a quick test with this patch - the associations and references I tested looked fine. Everything looked good on the read through as well. +1 -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
HE> # HG changeset patch HE> # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> 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@us.ibm.com
Dan Smith wrote:
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().
This makes sense to me, and I think it's a good suggestion. This also makes it clearer which piece is collecting references and which is collecting associators. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Dan Smith wrote:
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. [...] 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?
Thank you for your feedback. I will rework the patch as suggested (hopefully got it ;) ) and send out the patch. Thanks ... Heidi -- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert