9:40 a.m.
# HG changeset patch
# User Dan Smith <danms(a)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(a)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, \
11:44 a.m.
Dan Smith wrote:
# HG changeset patch
# User Dan Smith <danms(a)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(a)us.ibm.com>
I think this is a good compromise - looks good. +1
--
Kaitlin Rupert
IBM Linux Technology Center
karupert(a)us.ibm.com
5:38 a.m.
Kaitlin Rupert wrote:
Dan Smith wrote:
> # HG changeset patch
> # User Dan Smith <danms(a)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(a)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(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:42 a.m.
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(a)us.ibm.com
6210
days inactive
6216
days old
3 comments
3 participants
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert