[PATCH 0 of 3] Fixes to RegisteredProfile and ECTP depending on connect_by_classname approach
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1195812344 -3600 # Node ID a479cec9721a58c537929206bc5190b22f709fc1 # Parent 4cce597d4e48b5e3efc6e4307be3234bd3051c9b Enumeration of RegisteredProfile class is returning wrong instances Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 4cce597d4e48 -r a479cec9721a src/Virt_RegisteredProfile.c --- a/src/Virt_RegisteredProfile.c Fri Nov 23 10:57:30 2007 +0100 +++ b/src/Virt_RegisteredProfile.c Fri Nov 23 11:05:44 2007 +0100 @@ -43,25 +43,23 @@ CMPIInstance *reg_prof_instance(const CM CMPIInstance *reg_prof_instance(const CMPIBroker *broker, const char *namespace, const char **properties, + virConnectPtr conn, struct reg_prof *profile) { CMPIStatus s = {CMPI_RC_OK, NULL}; - CMPIObjectPath *op; CMPIInstance *instance = NULL; - char *classname; - - classname = get_typed_class("Xen", "RegisteredProfile"); - if (classname == NULL) { - goto out; - } - - op = CMNewObjectPath(broker, namespace, classname, &s); - if ((s.rc != CMPI_RC_OK) || CMIsNullObject(op)) - goto out; - - instance = CMNewInstance(broker, op, &s); - if ((s.rc != CMPI_RC_OK) || CMIsNullObject(op)) - goto out; + + instance = get_typed_instance(broker, + pfx_from_conn(conn), + "RegisteredProfile", + namespace); + + if (instance == NULL) { + CMSetStatusWithChars(broker, &s, + CMPI_RC_ERR_FAILED, + "Can't create HostSystem instance."); + goto out; + } if (properties) { s = CMSetPropertyFilter(instance, properties, NULL); @@ -83,8 +81,6 @@ CMPIInstance *reg_prof_instance(const CM (CMPIValue *)profile->reg_version, CMPI_chars); out: - free(classname); - return instance; } @@ -95,17 +91,23 @@ static CMPIStatus enum_profs(const CMPIO { CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *instance; + virConnectPtr conn = NULL; int i; + + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); + if (conn == NULL) + goto out; for (i = 0; profiles[i] != NULL; i++) { instance = reg_prof_instance(_BROKER, NAMESPACE(ref), - properties, + properties, + conn, profiles[i]); if (instance == NULL) { CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, "Can't create profile instance."); - goto out; + goto err; } if (names_only) @@ -114,6 +116,8 @@ static CMPIStatus enum_profs(const CMPIO CMReturnInstance(results, instance); } + err: + virConnectClose(conn); out: return s; } @@ -124,15 +128,20 @@ static CMPIStatus get_prof(const CMPIObj { CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *instance = NULL; + virConnectPtr conn = NULL; char* id; int i; + + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); + if (conn == NULL) + goto out; id = cu_get_str_path(ref, "InstanceID"); if (id == NULL) { CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, "No InstanceID specified"); - return s; + goto out; } for (i = 0; profiles[i] != NULL; i++) { @@ -140,18 +149,19 @@ static CMPIStatus get_prof(const CMPIObj instance = reg_prof_instance(_BROKER, NAMESPACE(ref), properties, + conn, profiles[i]); break; } } + free(id); + out: if(instance) CMReturnInstance(results, instance); else CMSetStatus(&s, CMPI_RC_ERR_NOT_FOUND); - - free(id); return s; } diff -r 4cce597d4e48 -r a479cec9721a src/Virt_RegisteredProfile.h --- a/src/Virt_RegisteredProfile.h Fri Nov 23 10:57:30 2007 +0100 +++ b/src/Virt_RegisteredProfile.h Fri Nov 23 11:05:44 2007 +0100 @@ -24,6 +24,7 @@ CMPIInstance *reg_prof_instance(const CM CMPIInstance *reg_prof_instance(const CMPIBroker *broker, const char *namespace, const char **properties, + virConnectPtr conn, struct reg_prof *profile); #endif
Heidi Eckhart wrote:
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1195812344 -3600 # Node ID a479cec9721a58c537929206bc5190b22f709fc1 # Parent 4cce597d4e48b5e3efc6e4307be3234bd3051c9b Enumeration of RegisteredProfile class is returning wrong instances Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com>
The move to connect_by_classname() work looks good, and in general I like the cleanup of reg_prof_instance(), but I have one question about it. You added a virConnectPtr to the argument list, and I'll grant the function does need a connection it doesn't already have in there, by why not pass in a CMPIObjectPath so that it used to make the connection? That way all the callers don't have to bother creating their own connections just to pass them in, and since reg_prof_instance is already being given a CMPIBroker and creates its own CMPIStatus, giving it the ref will give it everything it needs to make the appropriate connection and do the same "if (conn == NULL) goto out;" that the calling functions do right now. -- -Jay
The move to connect_by_classname() work looks good, and in general I like the cleanup of reg_prof_instance(), but I have one question about it. You added a virConnectPtr to the argument list, and I'll grant the function does need a connection it doesn't already have in there, I have reviewed the code before this change and all function do already have this connection pointer. They even have to have this connection, as
Jay Gagnon wrote: this is now the provider entry point - first check is if the requested class is equal to the installed hypervisor. So this is a performance optimization, as it avoids establishing a second connection to libvirt, while there is already one that can be used.
by why not pass in a CMPIObjectPath so that it used to make the connection? That way all the callers don't have to bother creating their own connections just to pass them in, and since reg_prof_instance is already being given a CMPIBroker and creates its own CMPIStatus, giving it the ref will give it everything it needs to make the appropriate connection and do the same "if (conn == NULL) goto out;" that the calling functions do right now.
-- 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
Heidi Eckhart wrote:
Jay Gagnon wrote:
The move to connect_by_classname() work looks good, and in general I like the cleanup of reg_prof_instance(), but I have one question about it. You added a virConnectPtr to the argument list, and I'll grant the function does need a connection it doesn't already have in there, I have reviewed the code before this change and all function do already have this connection pointer. They even have to have this connection, as this is now the provider entry point - first check is if the requested class is equal to the installed hypervisor. So this is a performance optimization, as it avoids establishing a second connection to libvirt, while there is already one that can be used.
Okay, good point. There are a couple of callers -- as shown by this patch :) -- that don't actually have a connection already, but they are the vast minority, and the extra connection is something we should avoid. -- -Jay
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1195812346 -3600 # Node ID 50b0c2bad1e31abff47a9c3a98f9e14b5e29efe9 # Parent a479cec9721a58c537929206bc5190b22f709fc1 Adoption of changes to RegisteredProfile Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r a479cec9721a -r 50b0c2bad1e3 src/Virt_ElementConformsToProfile.c --- a/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:05:44 2007 +0100 +++ b/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:05:46 2007 +0100 @@ -133,9 +133,14 @@ static CMPIStatus elem_to_prof(const CMP { CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *instance; + virConnectPtr conn = NULL; + struct reg_prof *candidate; char *classname; - struct reg_prof *candidate; int i; + + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); + if (conn == NULL) + goto out; classname = class_base_name(CLASSNAME(ref)); if (classname == NULL) { @@ -151,10 +156,12 @@ static CMPIStatus elem_to_prof(const CMP instance = reg_prof_instance(_BROKER, "/root/interop", - NULL, + NULL, + conn, candidate); if (instance == NULL) { - CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, + CMSetStatusWithChars(_BROKER, &s, + CMPI_RC_ERR_FAILED, "Can't create profile instance."); goto error; }
HE> diff -r a479cec9721a -r 50b0c2bad1e3 src/Virt_ElementConformsToProfile.c HE> --- a/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:05:44 2007 +0100 HE> +++ b/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:05:46 2007 +0100 HE> @@ -133,9 +133,14 @@ static CMPIStatus elem_to_prof(const CMP HE> { HE> CMPIStatus s = {CMPI_RC_OK, NULL}; HE> CMPIInstance *instance; HE> + virConnectPtr conn = NULL; You add a virConnectPtr here, HE> + struct reg_prof *candidate; HE> char *classname; HE> - struct reg_prof *candidate; HE> int i; HE> + HE> + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); HE> + if (conn == NULL) HE> + goto out; and open it here, HE> classname = class_base_name(CLASSNAME(ref)); HE> if (classname == NULL) { HE> @@ -151,10 +156,12 @@ static CMPIStatus elem_to_prof(const CMP HE> instance = reg_prof_instance(_BROKER, HE> "/root/interop", HE> - NULL, HE> + NULL, HE> + conn, HE> candidate); HE> if (instance == NULL) { HE> - CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, HE> + CMSetStatusWithChars(_BROKER, &s, HE> + CMPI_RC_ERR_FAILED, HE> "Can't create profile instance."); HE> goto error; HE> } But I don't see you close it anywhere before you exit the function, so I think this leaks the connection. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
But I don't see you close it anywhere before you exit the function, so I think this leaks the connection.
Dan Smith wrote: thanks ... fixed -- 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
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1195812682 -3600 # Node ID 883d767c64256f4d582f06125e05f392454f8a15 # Parent 50b0c2bad1e31abff47a9c3a98f9e14b5e29efe9 Fix class prefix to depend on established hypervisor connection Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 50b0c2bad1e3 -r 883d767c6425 src/Virt_ElementConformsToProfile.c --- a/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:05:46 2007 +0100 +++ b/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:11:22 2007 +0100 @@ -50,7 +50,8 @@ static CMPIStatus elem_instances(const C static CMPIStatus elem_instances(const CMPIObjectPath *ref, struct std_assoc_info *info, struct inst_list *list, - struct reg_prof *profile) + struct reg_prof *profile, + virConnectPtr conn) { CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIObjectPath *op; @@ -58,7 +59,8 @@ static CMPIStatus elem_instances(const C CMPIData data ; char *classname; - classname = get_typed_class("Xen", profile->provider_name); + classname = get_typed_class(pfx_from_conn(conn), + profile->provider_name); if (classname == NULL) { CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, @@ -101,9 +103,14 @@ static CMPIStatus prof_to_elem(const CMP struct inst_list *list) { CMPIStatus s = {CMPI_RC_OK, NULL}; + virConnectPtr conn = NULL; char *id; int i; + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); + if (conn == NULL) + goto out; + id = cu_get_str_path(ref, "InstanceID"); if (id == NULL) { CMSetStatusWithChars(_BROKER, &s, @@ -114,7 +121,8 @@ static CMPIStatus prof_to_elem(const CMP for (i = 0; profiles[i] != NULL; i++) { if (STREQ(id, profiles[i]->reg_id)) { - s = elem_instances(ref, info, list, profiles[i]); + s = elem_instances(ref, info, list, + profiles[i], conn); if ((s.rc != CMPI_RC_OK)) goto error; break; @@ -122,6 +130,7 @@ static CMPIStatus prof_to_elem(const CMP } error: + virConnectClose(conn); free(id); out: return s; @@ -180,10 +189,16 @@ static CMPIInstance *make_ref(const CMPI struct std_assoc_info *info, struct std_assoc *assoc) { - CMPIInstance *assoc_inst; + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *assoc_inst = NULL; + virConnectPtr conn = NULL; + + conn = connect_by_classname(_BROKER, CLASSNAME(source_op), &s); + if (conn == NULL) + goto out; assoc_inst = get_typed_instance(_BROKER, - "Xen", + pfx_from_conn(conn), "ElementConformsToProfile", NAMESPACE(source_op)); @@ -197,6 +212,9 @@ static CMPIInstance *make_ref(const CMPI (CMPIValue *)&(target_op), CMPI_ref); } + virConnectClose(conn); + + out: return assoc_inst; }
HE> diff -r 50b0c2bad1e3 -r 883d767c6425 src/Virt_ElementConformsToProfile.c HE> --- a/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:05:46 2007 +0100 HE> +++ b/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:11:22 2007 +0100 HE> @@ -50,7 +50,8 @@ static CMPIStatus elem_instances(const C HE> static CMPIStatus elem_instances(const CMPIObjectPath *ref, HE> struct std_assoc_info *info, HE> struct inst_list *list, HE> - struct reg_prof *profile) HE> + struct reg_prof *profile, HE> + virConnectPtr conn) Since we have a ref here, can we avoid passing a connection in and do the connect_by_classname() inside? -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> diff -r 50b0c2bad1e3 -r 883d767c6425 src/Virt_ElementConformsToProfile.c HE> --- a/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:05:46 2007 +0100 HE> +++ b/src/Virt_ElementConformsToProfile.c Fri Nov 23 11:11:22 2007 +0100 HE> @@ -50,7 +50,8 @@ static CMPIStatus elem_instances(const C HE> static CMPIStatus elem_instances(const CMPIObjectPath *ref, HE> struct std_assoc_info *info, HE> struct inst_list *list, HE> - struct reg_prof *profile) HE> + struct reg_prof *profile, HE> + virConnectPtr conn)
Since we have a ref here, can we avoid passing a connection in and do the connect_by_classname() inside? That does not make sense, as elem_instances() is an internal function only called by the external function prof_to_elem() and the connection was (and must be) already established by prof_to_elem(). So this would establish a second - unnecessary - connection to libvirt.
-- 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
HE> So this would establish a second - unnecessary - connection to HE> libvirt. Okay, good call. Thanks! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon