6:01 a.m.
# HG changeset patch
# User Heidi Eckhart <heidieck(a)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(a)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[] = {
3:47 p.m.
Heidi Eckhart wrote:
# HG changeset patch
# User Heidi Eckhart <heidieck(a)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(a)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(a)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
4:13 p.m.
JG> I realize it's a more logical abbreviation than "ref", but we use
JG> "ref" in so many places that I think it's pretty firmly
JG> established
For the parameter, I think source_ref would be fine. I don't think
changing it to not include "ref" makes it more clear, since the
convention (as it seems to me) is that the object path you pass in as
the "subject" of an operation is called the "reference".
For the "refinst" local variable, I think leaving "ref" in the name
also makes sense. The goal of the function is to "make a reference",
so "refinst" seems to be a reasonable name :)
JG> I personally always see "op" and think "operand" or
"operation." I
JG> would recommend a compromise of "source_ref" for that one.
Me too! I'd be okay with "source_ref", but I question how much more
clarity it will bring given how much patch churn it will produce.
However, if others want this change, I'm okay with it.
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
6:27 a.m.
Dan Smith wrote:
JG> I realize it's a more logical abbreviation than
"ref", but we use
JG> "ref" in so many places that I think it's pretty firmly
JG> established
For the parameter, I think source_ref would be fine. I don't think
changing it to not include "ref" makes it more clear, since the
convention (as it seems to me) is that the object path you pass in as
the "subject" of an operation is called the "reference".
For the "refinst" local variable, I think leaving "ref" in the name
also makes sense. The goal of the function is to "make a reference",
so "refinst" seems to be a reasonable name :)
JG> I personally always see "op" and think "operand" or
"operation." I
JG> would recommend a compromise of "source_ref" for that one.
Me too! I'd be okay with "source_ref", but I question how much more
clarity it will bring given how much patch churn it will produce.
However, if others want this change, I'm okay with it.
Thanks for the comments.
I was resending the patch as "variable renaming
in make_ref() of associations".
Yes, its very good to question this patch. And the more I think about,
the more I find, that there is the possibility to generalize the content
of make_ref() - because at the end most of the association's make_ref()
will look the same, besides the different association class names. I
will send out the patch for this idea.
--
Regards
Heidi Eckhart
Software Engineer
Linux Technology Center - Open Hypervisor
heidieck(a)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
6:37 a.m.
Jay Gagnon wrote:
Heidi Eckhart wrote:
> # HG changeset patch
> # User Heidi Eckhart <heidieck(a)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(a)linux.vnet.ibm.com>
>
<...>
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.
Sorry for causing this confusion. The renaming was one thing, but as the
patch showed how the code for each make_ref() would look like, I wanted
to also bring up this idea. But not very well described. I will send out
a patch "#2 - variable renaming in make_ref() of associations", which
shows what I meant.
--
Regards
Heidi Eckhart
Software Engineer
Linux Technology Center - Open Hypervisor
heidieck(a)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
6226
days inactive
6228
days old
4 comments
3 participants
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon