[PATCH 0 of 3] Reworked - Fixes to RegisteredProfile and ECTP depending on connect_by_classname approach.
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1196156619 -3600 # Node ID c3f590ec1553352439cbc6a884817ac13dc8eb40 # Parent ce846c470102c9c126b027c68d58f5205cadfb10 Enumeration of RegisteredProfile class is returning wrong instances Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r ce846c470102 -r c3f590ec1553 src/Virt_RegisteredProfile.c --- a/src/Virt_RegisteredProfile.c Tue Nov 27 10:34:13 2007 +0100 +++ b/src/Virt_RegisteredProfile.c Tue Nov 27 10:43:39 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) { + + 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; } - - 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; 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,12 +91,18 @@ 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) + return s; 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, @@ -115,6 +117,8 @@ static CMPIStatus enum_profs(const CMPIO } out: + virConnectClose(conn); + 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) + return s; 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,6 +149,7 @@ static CMPIStatus get_prof(const CMPIObj instance = reg_prof_instance(_BROKER, NAMESPACE(ref), properties, + conn, profiles[i]); break; } @@ -150,8 +160,10 @@ static CMPIStatus get_prof(const CMPIObj else CMSetStatus(&s, CMPI_RC_ERR_NOT_FOUND); - free(id); + + out: + virConnectClose(conn); return s; } diff -r ce846c470102 -r c3f590ec1553 src/Virt_RegisteredProfile.h --- a/src/Virt_RegisteredProfile.h Tue Nov 27 10:34:13 2007 +0100 +++ b/src/Virt_RegisteredProfile.h Tue Nov 27 10:43:39 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:
+ + 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;
I think the return message is incorrect here. You're creating attempting to create a RegisteredProfile instance, not a HostSystem instance. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Kaitlin Rupert wrote:
Heidi Eckhart wrote:
+ + 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;
I think the return message is incorrect here. You're creating attempting to create a RegisteredProfile instance, not a HostSystem instance.
very good eyes and a very good catch :) ... 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
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1196157587 -3600 # Node ID 690413b4aef454d03ce31003feea55a65fa5347b # Parent c3f590ec1553352439cbc6a884817ac13dc8eb40 Adoption of changes to RegisteredProfile Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r c3f590ec1553 -r 690413b4aef4 src/Virt_ElementConformsToProfile.c --- a/src/Virt_ElementConformsToProfile.c Tue Nov 27 10:43:39 2007 +0100 +++ b/src/Virt_ElementConformsToProfile.c Tue Nov 27 10:59:47 2007 +0100 @@ -133,9 +133,14 @@ static CMPIStatus elem_to_prof(const CMP { CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *instance; + virConnectPtr conn = NULL; char *classname; struct reg_prof *candidate; int i; + + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); + if (conn == NULL) + return s; 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; } @@ -165,6 +172,8 @@ static CMPIStatus elem_to_prof(const CMP error: free(classname); out: + virConnectClose(conn); + return s; }
Heidi Eckhart wrote:
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1196157587 -3600 # Node ID 690413b4aef454d03ce31003feea55a65fa5347b # Parent c3f590ec1553352439cbc6a884817ac13dc8eb40 Adoption of changes to RegisteredProfile Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com>
if (instance == NULL) { - CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, + CMSetStatusWithChars(_BROKER, &s, + CMPI_RC_ERR_FAILED, "Can't create profile instance.");
Not sure why this hadn't occurred to me earlier, and I don't know if this has been brought up already, but now that we have cu_statusf, should we use that instead of CMSetStatusWithChars even in places where we don't need any fancy formatting? It helps to maintain consistency, and (although this is a bit of a minor point), it is a substantially shorter function name. I believe the calls are the exact same when you don't have any formatting to do.
goto error; } @@ -165,6 +172,8 @@ static CMPIStatus elem_to_prof(const CMP error: free(classname); out: + virConnectClose(conn); + return s; }
Paying more attention to how we free things now that we know virConnectClose is fine when given a NULL, wouldn't the same hold for classname? As long as it is initialized to NULL when it is declared, it can always be freed at the end like conn. I realize that classname was already like that before your patch (I think I wrote it that way actually), but while your fixing this function up it seems like a nice housekeeping thing. Optionally, we can go with "not relevant to the patch" and I'll try and clean up this type of thing wherever I can find it as a separate patch. -- -Jay
JG> now that we have cu_statusf, should we use that instead of JG> CMSetStatusWithChars even in places where we don't need any fancy JG> formatting? Yes please! JG> It helps to maintain consistency, and (although this is a bit of a JG> minor point), it is a substantially shorter function name. It's not a minor point to me :) JG> Paying more attention to how we free things now that we know JG> virConnectClose is fine when given a NULL, wouldn't the same hold JG> for classname? Yes. Most of the stuff I write free()'s all the dynamic strings on exit (error or not) to make sure we don't skip one due to a broken exit sequence. JG> Optionally, we can go with "not relevant to the patch" and I'll JG> try and clean up this type of thing wherever I can find it as a JG> separate patch. Sounds good to me. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> now that we have cu_statusf, should we use that instead of JG> CMSetStatusWithChars even in places where we don't need any fancy JG> formatting?
Yes please!
JG> It helps to maintain consistency, and (although this is a bit of a JG> minor point), it is a substantially shorter function name.
It's not a minor point to me :)
JG> Paying more attention to how we free things now that we know JG> virConnectClose is fine when given a NULL, wouldn't the same hold JG> for classname?
Yes. Most of the stuff I write free()'s all the dynamic strings on exit (error or not) to make sure we don't skip one due to a broken exit sequence.
JG> Optionally, we can go with "not relevant to the patch" and I'll JG> try and clean up this type of thing wherever I can find it as a JG> separate patch.
Sounds good to me.
Okay, I can add these two items as to my list. After all the alloc cap and sdc_rasd stuff, I could go for a few things that require a little less heavy lifting. :) -- -Jay
Jay Gagnon wrote:
Dan Smith wrote:
JG> Paying more attention to how we free things now that we know JG> virConnectClose is fine when given a NULL, wouldn't the same hold JG> for classname?
Yes. Most of the stuff I write free()'s all the dynamic strings on exit (error or not) to make sure we don't skip one due to a broken exit sequence.
This is now fixed in an additional patch to this series for association ECTP. Nothing like that found in RegisteredProfile. As I saw that jay has already created a patch for the Status messages, I haven't touched this part - but fixed the issue Kaitlin found.
-- 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 1196157755 -3600 # Node ID 103e8269951254840e9a62239079d1916291dad9 # Parent 690413b4aef454d03ce31003feea55a65fa5347b Fix class prefix to depend on established hypervisor connection Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 690413b4aef4 -r 103e82699512 src/Virt_ElementConformsToProfile.c --- a/src/Virt_ElementConformsToProfile.c Tue Nov 27 10:59:47 2007 +0100 +++ b/src/Virt_ElementConformsToProfile.c Tue Nov 27 11:02:35 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) + return s; + 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; @@ -124,6 +132,8 @@ static CMPIStatus prof_to_elem(const CMP error: free(id); out: + virConnectClose(conn); + return s; } @@ -182,10 +192,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) + return NULL; assoc_inst = get_typed_instance(_BROKER, - "Xen", + pfx_from_conn(conn), "ElementConformsToProfile", NAMESPACE(source_op)); @@ -199,6 +215,9 @@ static CMPIInstance *make_ref(const CMPI (CMPIValue *)&(target_op), CMPI_ref); } + virConnectClose(conn); + + out: return assoc_inst; }
please ignore this one ... compile break because of warning: Virt_ElementConformsToProfile.c: In function 'make_ref': Virt_ElementConformsToProfile.c:220: warning: label 'out' defined but not used -- 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 (4)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon -
Kaitlin Rupert