6:29 a.m.
# HG changeset patch
# User Dan Smith <danms(a)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(a)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, \
7:38 a.m.
Dan Smith wrote:
# HG changeset patch
# User Dan Smith <danms(a)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(a)us.ibm.com
7:43 a.m.
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(a)us.ibm.com
5:59 p.m.
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(a)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
9:53 p.m.
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(a)us.ibm.com
9:54 a.m.
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(a)us.ibm.com
10:14 a.m.
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(a)us.ibm.com
6207
days inactive
6211
days old
6 comments
3 participants
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert