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

HE> I suggest to use STREQC to become more flexible to the requesters HE> case usage decision. Okay, good idea. 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