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

[Dan Smith]

HE> # HG changeset patch
HE> # User Heidi Eckhart <heidieck at 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 at us.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvirt-cim/attachments/20071211/28729945/attachment.sig>