[PATCH] Change cu_compare_ref() to be a little lighter-weight
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1194453648 28800 # Node ID 926009fc62a0080bcf93aaddf098b074ce71a096 # Parent 404c4803b1b542676c4d283226a7cc3b7f3ab58d Change cu_compare_ref() to be a little lighter-weight Now, we just check the keys that were provided by the client (or the CIMOM if that ever starts to happen). We will have already checked the required keys to actually do the lookup in the provider, which serves the "required" key purpose. Changes to libvirt-cim to follow shortly... Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 404c4803b1b5 -r 926009fc62a0 instance_util.c --- a/instance_util.c Fri Nov 02 15:29:38 2007 -0700 +++ b/instance_util.c Wed Nov 07 08:40:48 2007 -0800 @@ -88,38 +88,48 @@ static bool _compare_data(const CMPIData return false; } -const struct cu_property *cu_compare_ref(const CMPIObjectPath *ref, - const CMPIInstance *inst, - const struct cu_property *props) +const char *cu_compare_ref(const CMPIObjectPath *ref, + const CMPIInstance *inst) { - const struct cu_property *p = NULL; int i; CMPIStatus s; + int count; + const char *prop = NULL; - for (i = 0; props[i].name != NULL; i++) { + count = CMGetKeyCount(ref, &s); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Unable to get key count"); + return NULL; + } + + for (i = 0; i < count; i++) { CMPIData kd, pd; + CMPIString *str; - p = &props[i]; - - kd = CMGetKey(ref, p->name, &s); + kd = CMGetKeyAt(ref, i, &str, &s); if (s.rc != CMPI_RC_OK) { - if (p->required) - goto out; - else - continue; + CU_DEBUG("Failed to get key %i", i); + goto out; } - pd = CMGetProperty(inst, p->name, &s); - if (s.rc != CMPI_RC_OK) + prop = CMGetCharPtr(str); + CU_DEBUG("Comparing key `%s'", prop); + + pd = CMGetProperty(inst, prop, &s); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Failed to get property `%s'", prop); goto out; + } - if (!_compare_data(&kd, &pd)) + if (!_compare_data(&kd, &pd)) { + CU_DEBUG("No data match for `%s'", prop); goto out; + } } - p = NULL; + prop = NULL; out: - return p; + return prop; } /* diff -r 404c4803b1b5 -r 926009fc62a0 libcmpiutil.h --- a/libcmpiutil.h Fri Nov 02 15:29:38 2007 -0700 +++ b/libcmpiutil.h Wed Nov 07 08:40:48 2007 -0800 @@ -337,26 +337,17 @@ void inst_list_free(struct inst_list *li */ int inst_list_add(struct inst_list *list, CMPIInstance *inst); -struct cu_property { - const char *name; - bool required; -}; - /** * Compare key values in a reference to properties in an instance, - * making sure they are identical. If props identifies a particular - * key as not required, then absence in the object path will not - * result in failure of this test. + * making sure they are identical. * * @param ref The ObjectPath to examine * @param inst The Instance to compare - * @param props A NULL-terminated list of properties to compare - * @returns A pointer to the property structure of the first - * non-matching property, or NULL if all match - */ -const struct cu_property *cu_compare_ref(const CMPIObjectPath *ref, - const CMPIInstance *inst, - const struct cu_property *props); + * @returns A pointer to the name of the first non-matching property, + * or NULL if all match + */ +const char *cu_compare_ref(const CMPIObjectPath *ref, + const CMPIInstance *inst); #define DEFAULT_EIN(pn) \ static CMPIStatus pn##EnumInstanceNames(CMPIInstanceMI *self, \
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1194453648 28800 # Node ID 926009fc62a0080bcf93aaddf098b074ce71a096 # Parent 404c4803b1b542676c4d283226a7cc3b7f3ab58d Change cu_compare_ref() to be a little lighter-weight Now, we just check the keys that were provided by the client (or the CIMOM if that ever starts to happen). We will have already checked the required keys to actually do the lookup in the provider, which serves the "required" key purpose. Changes to libvirt-cim to follow shortly...
Signed-off-by: Dan Smith <danms@us.ibm.com>
I think this is a good compromise - looks good. +1 -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Kaitlin Rupert wrote:
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1194453648 28800 # Node ID 926009fc62a0080bcf93aaddf098b074ce71a096 # Parent 404c4803b1b542676c4d283226a7cc3b7f3ab58d Change cu_compare_ref() to be a little lighter-weight Now, we just check the keys that were provided by the client (or the CIMOM if that ever starts to happen). We will have already checked the required keys to actually do the lookup in the provider, which serves the "required" key purpose. Changes to libvirt-cim to follow shortly...
Signed-off-by: Dan Smith <danms@us.ibm.com>
I think this is a good compromise - looks good. +1
I agree to Kaitlin, that this is a good compromise. I had discussions with some CIM specialists and we concluded that currently a strict definition - on how the key values have to be set and checked - can not be found in one of the DMTF Spec's. The only clear statement is, that the sum of all key properties form the unique instance identifier. An agreement was also, that the provider should only accept these object paths (in regard to the number of key properties and their values), that the provider itself delivers via an enumeration. The current implementation includes the - more academical - possibility that a client generates an object path by itself, which is definitely a controversial and not recommended approach. This is not problematic as long as the client takes care of all key properties. Only the case that key properties are not handed over by the client breaks interoperability. But this could be fixed by a slight switch in the cu_compare_ref() function. Instead of getting the key count out of the reference it could be retrieved out of the instance : diff -r 6f09834081cf instance_util.c --- a/instance_util.c Mon Nov 12 11:03:04 2007 -0800 +++ b/instance_util.c Tue Nov 13 12:36:24 2007 +0100 @@ -118,8 +118,13 @@ const char *cu_compare_ref(const CMPIObj CMPIStatus s; int count; const char *prop = NULL; + CMPIObjectPath *op; - count = CMGetKeyCount(ref, &s); + op = CMGetObjectPath(inst, &s); + if ((op == NULL) || (s.rc != CMPI_RC_OK)) + return NULL; + + count = CMGetKeyCount(op, &s); if (s.rc != CMPI_RC_OK) { CU_DEBUG("Unable to get key count"); return NULL; The reason why the CIMOMs do not check the key properties (and accept not set key properties ) might be, that no CIM spec does definitely clarify the responsibilities here. So the CIMOM hands it over to the provider. Which makes sense as the provider is the one with the implementation logic for the key generation and always needs a way to identify its real instances out of a given object path. -- 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> The current implementation includes the - more academical - HE> possibility that a client generates an object path by itself, HE> which is definitely a controversial and not recommended HE> approach. This is not problematic as long as the client takes care HE> of all key properties. Only the case that key properties are not HE> handed over by the client breaks interoperability. Well, if all of the other key properties are unrelated to the output, I don't see how it matters. In most cases, we have one true key and the rest are all identical. If the client fashioned an object path with two conflicting keys, then it would make sense to reject the request, and the current implementation would do so correctly. For the sake of human-driven manual testing, it seems reasonable to me to ensure that the client provided enough keys to adequately disambiguate the requested instance from any others. However, I can appreciate that this is not the common case and that a normal client should be using only object paths that we have previously generated. HE> The reason why the CIMOMs do not check the key properties (and HE> accept not set key properties ) might be, that no CIM spec does HE> definitely clarify the responsibilities here. So the CIMOM hands HE> it over to the provider. Which makes sense as the provider is the HE> one with the implementation logic for the key generation and HE> always needs a way to identify its real instances out of a given HE> object path. Right, which is why I originally had a list of required properties passed to this function. Because the CIMOM doesn't do its own checking, we have to do it. Since we know the domain, we should be able to be intelligent about which properties will always be identical, and which are really required for disambiguation. That said, your fix is small and easy, so if you want to write up a formal patch, I'll apply it. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert