# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1194042578 25200 # Node ID 0df048ad2e99c8eda6c843f148168a9fa0161399 # Parent 05b01d03f0b801afe266d8d012bc464be6a2d165 Add cu_compare_ref() This provides a generic way to easily compare a given ref against an instance, which should be useful in GetInstance functions for making sure the returned instance matches what was asked. Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 05b01d03f0b8 -r 0df048ad2e99 instance_util.c --- a/instance_util.c Sun Feb 15 22:01:53 2015 -0800 +++ b/instance_util.c Fri Nov 02 15:29:38 2007 -0700 @@ -19,6 +19,7 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ #include <stdbool.h> +#include <string.h> #include "libcmpiutil.h" @@ -68,6 +69,59 @@ unsigned int cu_return_instance_names(co return c; } +static bool _compare_data(const CMPIData *a, const CMPIData *b) +{ + if (a->type != b->type) + return false; + + if (a->type & CMPI_string) { + const char *as = CMGetCharPtr(a->value.string); + const char *bs = CMGetCharPtr(b->value.string); + + return STREQ(as, bs); + } else if (a->type & CMPI_INTEGER) { + return memcmp(&a->value, &b->value, sizeof(a->value)) == 0; + } + + CU_DEBUG("Unhandled CMPI type: `%i'", a->type); + + return false; +} + +const struct cu_property *cu_compare_ref(const CMPIObjectPath *ref, + const CMPIInstance *inst, + const struct cu_property *props) +{ + const struct cu_property *p = NULL; + int i; + CMPIStatus s; + + for (i = 0; props[i].name != NULL; i++) { + CMPIData kd, pd; + + p = &props[i]; + + kd = CMGetKey(ref, p->name, &s); + if (s.rc != CMPI_RC_OK) { + if (p->required) + goto out; + else + continue; + } + + pd = CMGetProperty(inst, p->name, &s); + if (s.rc != CMPI_RC_OK) + goto out; + + if (!_compare_data(&kd, &pd)) + goto out; + } + + p = NULL; + out: + return p; +} + /* * Local Variables: * mode: C diff -r 05b01d03f0b8 -r 0df048ad2e99 libcmpiutil.h --- a/libcmpiutil.h Sun Feb 15 22:01:53 2015 -0800 +++ b/libcmpiutil.h Fri Nov 02 15:29:38 2007 -0700 @@ -336,6 +336,27 @@ void inst_list_free(struct inst_list *li * @returns nonzero on success, zero on failure */ 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. + * + * @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); #define DEFAULT_EIN(pn) \ static CMPIStatus pn##EnumInstanceNames(CMPIInstanceMI *self, \
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1194042578 25200 # Node ID 0df048ad2e99c8eda6c843f148168a9fa0161399 # Parent 05b01d03f0b801afe266d8d012bc464be6a2d165 Add cu_compare_ref()
This provides a generic way to easily compare a given ref against an instance, which should be useful in GetInstance functions for making sure the returned instance matches what was asked.
+const struct cu_property *cu_compare_ref(const CMPIObjectPath *ref, + const CMPIInstance *inst, + const struct cu_property *props)
This looks good. Only minor issue is that you don't return the status, but I the other cu_<> functions behave the same way. +1 -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
KR> This looks good. Only minor issue is that you don't return the KR> status, but I the other cu_<> functions behave the same way. +1 I don't return a CMPIStatus, but I do return a pointer to the caller's cu_property struct that didn't pass the check, which they can use to set a CMPIStatus if they wish (see my subsequent patches to libvirt-cim). I did it this way to avoid having to pass around a CMPIBroker, and so that the caller has more control over the status value. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
+static bool _compare_data(const CMPIData *a, const CMPIData *b) +{ + if (a->type != b->type) + return false; + + if (a->type & CMPI_string) { + const char *as = CMGetCharPtr(a->value.string); + const char *bs = CMGetCharPtr(b->value.string); + + return STREQ(as, bs);
I suggest to use STREQC to become more flexible to the requesters case usage decision.
+ +const struct cu_property *cu_compare_ref(const CMPIObjectPath *ref, + const CMPIInstance *inst, + const struct cu_property *props) +{ + const struct cu_property *p = NULL; + int i; + CMPIStatus s; + + for (i = 0; props[i].name != NULL; i++) { + CMPIData kd, pd; + + p = &props[i]; + + kd = CMGetKey(ref, p->name, &s); + if (s.rc != CMPI_RC_OK) { + if (p->required) + goto out; + else + continue; + }
I fear these lines include a possible crash scenario. For the case that CMGetKey() failed, kd is NULL, but p->required is false - it continues. Later _compare_data() does not check the CMPIData pointer against NULL.
+ + pd = CMGetProperty(inst, p->name, &s); + if (s.rc != CMPI_RC_OK) + goto out; + + if (!_compare_data(&kd, &pd)) + goto out; + } + + p = NULL; + out: + return p; +}
Other than that I have a conceptual question. In this implementation the provider is responsible for defining the list of key properties and has the freedom to mark the properties to be required or not. CIM itself does not give a provider this level of freedom. All key properties of an object path (class) have to be set. And an instance is only then the requested instance, if all key properties fit to the values known to the provider. Otherwise the client requested another instance. The required flag is a very interesting approach, but we should remove it, to avoid misbehavior of providers. A freedom that is not given, can not be used ;). I would also think, that it is not necessary to let the provider define the list of all key properties. As I said - the list of key properties is fix and given with the class definition. The CIMOM checks if all key properties are set and refuses requests with NULL value key properties. A provider does not has to handle this. Now CMPI offers macros to implement a generic approach to iterate over the list of key properties without the knowledge of certain names: CMGetKeyCount() to retrieve the number of key properties iterate over the number of key properties and CMGetKeyAt() to retrieve the key properties at this index Now my suggestion is to implement this generic approach for cu_compare_ref() and remove the props list from the input parameter list. What do you think ? -- 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> I suggest to use STREQC to become more flexible to the requesters HE> case usage decision. Okay, good idea.
+ for (i = 0; props[i].name != NULL; i++) { + CMPIData kd, pd; + + p = &props[i]; + + kd = CMGetKey(ref, p->name, &s); + if (s.rc != CMPI_RC_OK) { + if (p->required) + goto out; + else + continue; + }
HE> I fear these lines include a possible crash scenario. For the case HE> that CMGetKey() failed, kd is NULL, but p->required is false - it HE> continues. Later _compare_data() does not check the CMPIData HE> pointer against NULL.
But continue goes to the next iteration of the for loop, where we'll either end, or reset kd to the next key value. I don't think there is a crash here. HE> Other than that I have a conceptual question. In this HE> implementation the provider is responsible for defining the list HE> of key properties and has the freedom to mark the properties to be HE> required or not. CIM itself does not give a provider this level of HE> freedom. All key properties of an object path (class) have to be HE> set. And an instance is only then the requested instance, if all HE> key properties fit to the values known to the provider. Otherwise HE> the client requested another instance. The required flag is a very HE> interesting approach, but we should remove it, to avoid HE> misbehavior of providers. A freedom that is not given, can not be HE> used ;). I know what you're saying, and I agree with you on a basic level. However, if one of the key properties is unique enough to identify an instance among all others, and the other keys are not specified (it would be different if they were specified and wrong) what practical sense does it make for us to check them? It makes testing harder, for sure. I think that the practical use of not checking meaningless keys is a reasonable optimization over the CIM spec, but I'm fine with either of the following three solutions: 1. Leaving it the way I have it, bending the spec intelligently 2. Modifying libcmpiutil to have a compile-time flag for relaxed key checking (for testing, debug, etc) 3. Removing the feature altogether and making all keys checked all the time Thoughts? HE> I would also think, that it is not necessary to let the provider HE> define the list of all key properties. As I said - the list of key HE> properties is fix and given with the class definition. Yes, but if you want some to be optional and some not (as I originally intended), then you need to provide a list of those that you care about. Perhaps this should have been a list of required keys, and let the routine check the other keys implicitly. HE> The CIMOM checks if all key properties are set and refuses HE> requests with NULL value key properties. A provider does not has HE> to handle this. That's not true, at least in the GetInstance case. I can do a GetInstance of, say, VSMS with only Name set. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
DS> I'm fine with either of the following three solutions: DS> 1. Leaving it the way I have it, bending the spec intelligently DS> 2. Modifying libcmpiutil to have a compile-time flag for relaxed key DS> checking (for testing, debug, etc) DS> 3. Removing the feature altogether and making all keys checked all DS> the time DS> Thoughts? So, after some experimentation, I have determined that neither Pegasus nor SFCB set the key properties in an ObjectPath if they are not set by the client. The following works on both: % wbemcli gi 'http://root:password@localhost/root/ibmsd:Xen_VirtualSystemManagementService.Name="Management Service"' This means that Pegasus does not require all keys (as specified in the schema for a given class) to be set. If I loop through the keys in the GetInstance handler case of above, only "Name" is set. I don't get the other three keys of CIM_Service set. If this level of checking is required for strict adherence, I really think the CIMOMs should be checking that the keys are specified and we should be making sure that keys match properties. So, my suggestion would be to either leave it the way it is, or remove the properties paramter and just check that whatever keys were specified match the associated properties. Anyone else have an opinion, based on this updated information? :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
So, after some experimentation, I have determined that neither Pegasus nor SFCB set the key properties in an ObjectPath if they are not set by the client. The following works on both:
% wbemcli gi 'http://root:password@localhost/root/ibmsd:Xen_VirtualSystemManagementService.Name="Management Service"'
This also worked for me using Pegaus.
This means that Pegasus does not require all keys (as specified in the schema for a given class) to be set.
If I loop through the keys in the GetInstance handler case of above, only "Name" is set. I don't get the other three keys of CIM_Service set.
Is there someplace that spells out exactly how the keys should be used? My only understanding of them is that the keys - as a set - should be unique per instance. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert