[Libvirt-cim] [PATCH] Add cu_compare_ref()

[Dan Smith]

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 at us.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvirt-cim/attachments/20071105/754bc9b5/attachment.sig>