[PATCH 0 of 3] #3 - Enhance std association logic for reference lists and input parameter handling
Please don't ask me what happened to the patch set before and why patch 3 was not submitted to the list. So resending the complete patch set once again ... and hope. This patch set fixes: - enhance the std_assoc struct to support lists for source, target and association classnames - fixed a wrong error return for a NULL handler - reworked check for input parameter: assocClass, resultClass, role, resultRole
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1196283440 -3600 # Node ID 85f916da818a0d078d21efbb09df724e9cbf31dc # Parent 074c037c19ce46559f36a3fd06b943f5fe3c6009 Enhance handling of association's references The source and target classnames of std_assoc are now lists, containing all supported classnames. This approach frees the provider from listing all possible combinations as instances of std_assoc. Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 074c037c19ce -r 85f916da818a std_association.c --- a/std_association.c Tue Nov 27 16:29:13 2007 -0500 +++ b/std_association.c Wed Nov 28 21:57:20 2007 +0100 @@ -61,18 +61,44 @@ static bool match_class(const CMPIBroker static bool match_class(const CMPIBroker *broker, const char *ns, const char *test_class, - const char *comp_class) + char **comp_class_list) { CMPIObjectPath *rop; + char *comp_class; + int i; rop = CMNewObjectPath(broker, ns, test_class, NULL); - if ((test_class == NULL) || - (comp_class == NULL) || - match_op(broker, rop, comp_class)) - return true; - else - return false; + for (i = 0; comp_class_list[i]; i++) { + comp_class = comp_class_list[i]; + + if ((test_class == NULL) || + (comp_class == NULL) || + match_op(broker, rop, comp_class)) + return true; + } + + return false; +} + +static bool match_source_class(const CMPIBroker *broker, + const CMPIObjectPath *ref, + struct std_assoc *ptr) +{ + char *source_class; + int i; + + for (i = 0; ptr->source_class[i]; i++) { + source_class = ptr->source_class[i]; + + if (CMClassPathIsA(broker, + ref, + source_class, + NULL)) + return true; + } + + return false; } static CMPIStatus filter_results(struct inst_list *list, @@ -113,13 +139,13 @@ std_assoc_get_handler(const struct std_a std_assoc_get_handler(const struct std_assoc_ctx *ctx, const CMPIObjectPath *ref) { - struct std_assoc *ptr; + struct std_assoc *ptr = NULL; int i; for (i = 0; ctx->handlers[i]; i++) { ptr = ctx->handlers[i]; - if (CMClassPathIsA(ctx->brkr, ref, ptr->source_class, NULL)) + if (match_source_class(ctx->brkr, ref, ptr)) return ptr; } diff -r 074c037c19ce -r 85f916da818a std_association.h --- a/std_association.h Tue Nov 27 16:29:13 2007 -0500 +++ b/std_association.h Wed Nov 28 21:57:20 2007 +0100 @@ -37,13 +37,13 @@ typedef CMPIInstance *(*make_ref_t)(cons struct std_assoc *); struct std_assoc { - char *source_class; + char **source_class; char *source_prop; - char *target_class; + char **target_class; char *target_prop; - char *assoc_class; + char **assoc_class; assoc_handler_t handler; make_ref_t make_ref;
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1196283441 -3600 # Node ID 53d512fbf05747b7bca5c189b0d6ab5e7e63f90b # Parent 85f916da818a0d078d21efbb09df724e9cbf31dc Removed error messages that cause wrong error returns The case that no handler was found for a request caused an error as return. This is wrong as the not-found handler tells only, that the provider is not responsible for the given request. In scenarios with a general request causing the call of several association providers that return valid instances, the former behavior causes a wrong result, as the CIMOM would return the error and revoke all valid results. Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 85f916da818a -r 53d512fbf057 std_association.c --- a/std_association.c Wed Nov 28 21:57:20 2007 +0100 +++ b/std_association.c Wed Nov 28 21:57:21 2007 +0100 @@ -170,9 +170,6 @@ static CMPIStatus do_assoc(struct std_as handler = std_assoc_get_handler(ctx, ref); if (handler == NULL) { CU_DEBUG("No handler found."); - cu_statusf(ctx->brkr, &s, - CMPI_RC_ERR_FAILED, - "Unable to handle this association"); goto out; } @@ -269,9 +266,7 @@ static CMPIStatus do_ref(struct std_asso handler = std_assoc_get_handler(ctx, ref); if (handler == NULL) { - cu_statusf(ctx->brkr, &s, - CMPI_RC_ERR_FAILED, - "Unable to handle this association"); + CU_DEBUG("No handler found."); goto out; }
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1196287649 -3600 # Node ID ae5641cb6977307772a38c2585be008fc72b59a1 # Parent 53d512fbf05747b7bca5c189b0d6ab5e7e63f90b Enhance handling of input parameter of std_association logic Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 53d512fbf057 -r ae5641cb6977 std_association.c --- a/std_association.c Wed Nov 28 21:57:21 2007 +0100 +++ b/std_association.c Wed Nov 28 23:07:29 2007 +0100 @@ -67,14 +67,17 @@ static bool match_class(const CMPIBroker char *comp_class; int i; - rop = CMNewObjectPath(broker, ns, test_class, NULL); - + if (test_class == NULL) + return true; + + if (comp_class_list == NULL) + return true; + for (i = 0; comp_class_list[i]; i++) { comp_class = comp_class_list[i]; - - if ((test_class == NULL) || - (comp_class == NULL) || - match_op(broker, rop, comp_class)) + rop = CMNewObjectPath(broker, ns, comp_class, NULL); + + if (CMClassPathIsA(broker, rop, test_class, NULL)) return true; } @@ -137,18 +140,83 @@ out: static struct std_assoc * std_assoc_get_handler(const struct std_assoc_ctx *ctx, + struct std_assoc_info *info, const CMPIObjectPath *ref) { struct std_assoc *ptr = NULL; int i; + bool rc; + + CU_DEBUG("Calling Provider: '%s'", + info->provider_name); for (i = 0; ctx->handlers[i]; i++) { ptr = ctx->handlers[i]; if (match_source_class(ctx->brkr, ref, ptr)) - return ptr; - } - + break; + } + + if (!ptr) + goto out; + + if (info->assoc_class) { + CU_DEBUG("Check client's assocClass: '%s'", + info->assoc_class); + + rc = match_class(ctx->brkr, + NAMESPACE(ref), + info->assoc_class, + ptr->assoc_class); + + if (!rc) { + CU_DEBUG("AssocClass not valid."); + goto out; + } + CU_DEBUG("AssocClass valid."); + } + + if (info->result_class) { + CU_DEBUG("Check client's resultClass: '%s'", + info->result_class); + + rc = match_class(ctx->brkr, + NAMESPACE(ref), + info->result_class, + ptr->target_class); + + if (!rc) { + CU_DEBUG("ResultClass not valid."); + goto out; + } + CU_DEBUG("ResultClass valid."); + } + + if (info->role) { + CU_DEBUG("Check client's role: '%s'", + info->role); + + if (!STREQC(info->role, ptr->source_prop)) { + CU_DEBUG("Role not valid."); + goto out; + } + CU_DEBUG("Role valid."); + } + + if (info->result_role) { + CU_DEBUG("Check client's resultRole: '%s'", + info->result_role); + + if (!STREQC(info->result_role, ptr->target_prop)) { + CU_DEBUG("ResultRole not valid."); + goto out; + } + CU_DEBUG("ResultRole valid."); + } + + return ptr; + + out: return NULL; } @@ -158,57 +226,21 @@ static CMPIStatus do_assoc(struct std_as const CMPIObjectPath *ref, bool names_only) { + CMPIStatus s = {CMPI_RC_OK, NULL}; struct inst_list list; - CMPIStatus s; struct std_assoc *handler; - bool rc; inst_list_init(&list); CU_DEBUG("Getting handler..."); - - handler = std_assoc_get_handler(ctx, ref); + handler = std_assoc_get_handler(ctx, info, ref); if (handler == NULL) { CU_DEBUG("No handler found."); goto out; } - - CU_DEBUG("OK.\n\tsource: '%s'\n\ttarget: '%s'\n\tassoc_class: '%s'", - handler->source_class, - handler->target_class, - handler->assoc_class); - CU_DEBUG("Calling match_class: \n\tinfo->result_class: '%s'", - info->result_class); - - rc = match_class(ctx->brkr, - NAMESPACE(ref), - info->result_class, - handler->target_class); - if (!rc) { - CU_DEBUG("Match_class failed."); - cu_statusf(ctx->brkr, &s, - CMPI_RC_ERR_FAILED, - "Result class is not valid for this association"); - goto out; - } - CU_DEBUG("Match_class succeeded."); - - CU_DEBUG("Calling match_class: \n\tinfo->assoc_class: '%s'", - info->assoc_class); - rc = match_class(ctx->brkr, - NAMESPACE(ref), - info->assoc_class, - handler->assoc_class); - if (!rc) { - CU_DEBUG("Match_class failed."); - cu_statusf(ctx->brkr, &s, - CMPI_RC_ERR_FAILED, - "Association class given is not valid for" - "this association"); - goto out; - } - CU_DEBUG("Match_class succeeded, calling handler->handler..."); - + CU_DEBUG("Getting handler succeeded."); + + CU_DEBUG("Calling handler->handler..."); s = handler->handler(ref, info, &list); if (s.rc != CMPI_RC_OK) { @@ -256,32 +288,22 @@ static CMPIStatus do_ref(struct std_asso const CMPIObjectPath *ref, bool names_only) { + CMPIStatus s = {CMPI_RC_OK, NULL}; struct inst_list list; - CMPIStatus s; - int i; struct std_assoc *handler; - bool rc; + int i; inst_list_init(&list); - handler = std_assoc_get_handler(ctx, ref); + CU_DEBUG("Getting handler..."); + handler = std_assoc_get_handler(ctx, info, ref); if (handler == NULL) { CU_DEBUG("No handler found."); goto out; } - - rc = match_class(ctx->brkr, - NAMESPACE(ref), - info->result_class, - handler->assoc_class); - if (!rc) { - cu_statusf(ctx->brkr, &s, - CMPI_RC_ERR_FAILED, - "Result class is not valid for this association"); - goto out; - } - - + CU_DEBUG("Getting handler succeeded."); + + CU_DEBUG("Calling handler->handler..."); s = handler->handler(ref, info, &list); if ((s.rc != CMPI_RC_OK) || (list.list == NULL)) goto out;
Heidi Eckhart wrote: Sorry for revisiting this patch. I failed to test references yesterday, so I didn't catch this problem then.
static struct std_assoc * std_assoc_get_handler(const struct std_assoc_ctx *ctx, + struct std_assoc_info *info, const CMPIObjectPath *ref) {
+ + if (info->assoc_class) { + CU_DEBUG("Check client's assocClass: '%s'", + info->assoc_class); + + rc = match_class(ctx->brkr, + NAMESPACE(ref), + info->assoc_class, + ptr->assoc_class); + + if (!rc) { + CU_DEBUG("AssocClass not valid."); + goto out; + } + CU_DEBUG("AssocClass valid."); + } + + if (info->result_class) { + CU_DEBUG("Check client's resultClass: '%s'", + info->result_class); + + rc = match_class(ctx->brkr, + NAMESPACE(ref), + info->result_class, + ptr->target_class); + + if (!rc) { + CU_DEBUG("ResultClass not valid."); + goto out; + } + CU_DEBUG("ResultClass valid."); + } This works for associations. But for references, the result class is
<snip> the association class name. So, when you call std_assoc_get_handler() from do_ref(), you end up checking the client's result class (which is the association name) against the result class list instead of the association list. Here's some sample debug: std_association.c(152): Calling Provider: 'associationVirt_HostedDependencyProvider' std_association.c(182): Check client's resultClass: 'Xen_HostedDependency' std_association.c(78): provider accepts Xen_ComputerSystem resultclass std_association.c(78): provider accepts KVM_ComputerSystem resultclass std_association.c(190): ResultClass not valid. std_association.c(302): No handler found. You could add flag argument to std_assoc_get_handler() that indicates whether the call is asking for references or an associators. Although, I'm not sure that's the most elegant way. Also, adding the debug messages that print which values the provider is checking the result class / assoc class / etc might be useful - that is, print out what classes the provider accepts. Someone else can chime in as to whether this would be useful. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
This works for associations. But for references, the result class is the association class name. So, when you call std_assoc_get_handler() from do_ref(), you end up checking the client's result class (which is the association name) against the result class list instead of the association list. Here's some sample debug:
std_association.c(152): Calling Provider: 'associationVirt_HostedDependencyProvider' std_association.c(182): Check client's resultClass: 'Xen_HostedDependency' std_association.c(78): provider accepts Xen_ComputerSystem resultclass std_association.c(78): provider accepts KVM_ComputerSystem resultclass std_association.c(190): ResultClass not valid. std_association.c(302): No handler found.
You could add flag argument to std_assoc_get_handler() that indicates whether the call is asking for references or an associators. Although, I'm not sure that's the most elegant way.
Also, adding the debug messages that print which values the provider is checking the result class / assoc class / etc might be useful - that is, print out what classes the provider accepts. Someone else can chime in as to whether this would be useful. That's also a very good catch :). You are great in indicating bugs in
Kaitlin Rupert wrote: the association logic :) ! Fortunately this can be fixed very easy - the assocClass and resultClass values have only been swapped while setting std_assoc_info values in stda_ReferenceNames() and stda_References(). In the case of reference calls the client's resultClass value needs to become the value for assocClass internally. @@ -373,8 +397,8 @@ CMPIStatus stda_ReferenceNames(CMPIAssoc const char *role) { struct std_assoc_info info = { - NULL, - resultClass, + resultClass , + NULL, role, NULL, NULL, @@ -394,8 +418,8 @@ CMPIStatus stda_References(CMPIAssociati const char **properties) { struct std_assoc_info info = { - NULL, resultClass, + NULL, role, NULL, properties, -- 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:
Please don't ask me what happened to the patch set before and why patch 3 was not submitted to the list. So resending the complete patch set once again ... and hope. This patch set fixes: - enhance the std_assoc struct to support lists for source, target and association classnames - fixed a wrong error return for a NULL handler - reworked check for input parameter: assocClass, resultClass, role, resultRole
The set looks good. I liked the added role, resultRole support. Also, I liked how you moved the checking of assocClass, resultClass, role, resultRole to std_assoc_get_handler() instead of do_assoc(). It seems like a more applicable place since std_assoc_get_handler() is supposed to determine whether the provider can handle the association, and if so, which function will be handling the call. Nice work =) +1 One odd thing I noticed about this patch set is that I could only get the ECTP association to build properly. It seems like the other associations need to be updated like ECTP before they will build properly. This is strange because I was able to build all of the associations with the first iteration of the patchset without problems. I didn't see anything obvious that would cause the change. It looks like all the providers will need to be updated at the same time the libcmpiutil pieces are checked in, which probably makes sense anyway. =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
KR> One odd thing I noticed about this patch set is that I could only get KR> the ECTP association to build properly. It seems like the other KR> associations need to be updated like ECTP before they will build KR> properly. Yeah, they'll all need a change for sure. KR> This is strange because I was able to build all of the KR> associations with the first iteration of the patchset without KR> problems. Maybe you didn't do a clean first? The libvirt-cim build system won't notice changes in the libcmpiutil stuff in order to force a clean rebuild. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert