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

[Heidi Eckhart]

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 at 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