[Libvirt-cim] [PATCH] [RFC] make_ref() of associations

[Jay Gagnon]

Heidi Eckhart wrote:
> # HG changeset patch
> # User Heidi Eckhart <heidieck at linux.vnet.ibm.com>
> # Date 1196686834 -3600
> # Node ID bea3e027d42ff41fc452935427981739bab76573
> # Parent  a1582d092f517919470b9ce7ff034b89e4b2bade
> [RFC] make_ref() of associations
>
> While thinking about the implementation of make_ref() and about
> my proposal to base the asscociation's instance creation on a
> connect_by_classname, I came to the conclusion that opening a
> connection to libvirt for not using it, is overkill for this
> method. Once make_ref() gets called by the std_association logic,
> the provider can rely on that the prefix of the given reference
> was checked for the right hypervisor prefix. So I suggest to
> update all make_ref() functions to use get_typed_instance().
> ... and refix some of my fixes :0.
> Besides that I suggest to rename some of the variables in
> make_ref() to make the relations clearer.
> Signed-off-by: Heidi Eckhart <heidieck at linux.vnet.ibm.com>
>
> diff -r a1582d092f51 -r bea3e027d42f src/Virt_ElementAllocatedFromPool.c
> --- a/src/Virt_ElementAllocatedFromPool.c	Mon Dec 03 13:49:01 2007 +0100
> +++ b/src/Virt_ElementAllocatedFromPool.c	Mon Dec 03 14:00:34 2007 +0100
> @@ -241,27 +241,27 @@ static CMPIStatus pool_to_vdev(const CMP
>          return s;
>  }
>
> -static CMPIInstance *make_ref(const CMPIObjectPath *ref,
> -                              const CMPIInstance *inst,
> +static CMPIInstance *make_ref(const CMPIObjectPath *source_op,
> +                              const CMPIInstance *target_inst,
>                                struct std_assoc_info *info,
>                                struct std_assoc *assoc)
>  {
> -        CMPIInstance *refinst = NULL;
> -
> -        refinst = get_typed_instance(_BROKER,
> -                                     CLASSNAME(ref),
> -                                     "ElementAllocatedFromPool",
> -                                     NAMESPACE(ref));
> -
> -        if (refinst != NULL) {
> -                CMPIObjectPath *instop;
> -
> -                instop = CMGetObjectPath(inst, NULL);
> -
> -                set_reference(assoc, refinst, ref, instop);
> -        }
> -
> -        return refinst;
> +        CMPIInstance *assoc_inst = NULL;
> +
> +        assoc_inst = get_typed_instance(_BROKER,
> +                                        CLASSNAME(source_op),
> +                                        "ElementAllocatedFromPool",
> +                                        NAMESPACE(source_op));
> +
> +        if (assoc_inst != NULL) {
> +                CMPIObjectPath *target_op;
> +
> +                target_op = CMGetObjectPath(target_inst, NULL);
> +
> +                set_reference(assoc, assoc_inst, source_op, target_op);
> +        }
> +
> +        return assoc_inst;
>  }
>
>  char* antecedent[] = {
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
>   
Couple of things here. First thing, I'm okay with renaming things to
make it more clear, but I'm not really a fan of using "op" for
ObjectPath. I realize it's a more logical abbreviation than "ref", but
we use "ref" in so many places that I think it's pretty firmly
established, and I personally always see "op" and think "operand" or
"operation." I would recommend a compromise of "source_ref" for that
one. Second thing, I think I'm missing something in the first paragraph
of your commit message. I'm not entirely sure I understand the
explanation that leads up to "So I suggest to update all make_ref()
functions to use get_typed_instance()," and it also doesn't seem to
really relate to this patch, which only appears to do some renaming in
make_ref. It seems like a good point of discussion, and clarification
would be good, but maybe we should move it to a separate thread and
limit this patch discussion to the variable naming.

-- 

-Jay