Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Chester Lin writes ("[PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> is defined in IDL.
> 
> The two enums are deliberately identical and IDL only exists so that
> libxl clients don't need to include libxc headers directly.
> 
> This change adds an explicit cast to fix the
> Coverity warning, and tweaks the surrounding code to more closely
> conform to the guidelines in CODING_STYLE.

I can see why Coverity is complaining.  I think, overall, that the
existing situation is not really desirable.


In fact there are not two but *three* of these enums:

 * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
 * enum xc_psr_cat_type (xenctrl.h)
 * Enumeration("psr_cbm_type",...) (libxl_types.idl)

xc_psr.c explicitly converts between the first two with a switch
statement.  libxl does no conversion.


I think our general rule is that enums from the hypervisor public
headers are OK to expose to libxl users, because that avoids a pile of
needless translation.  Ian, Wei, do you agree ?

Of course in this particular case, we shouldn't expect libxl users to
consume XEN_DOMCTL_*.  Instead, I would have expected
XEN_DOMCTL_PSR_CAT_OP_SET with a separate enum
XEN_PSR_CAT_L3_* or something.


With the current setup there is no mechanism (computer- or
human-mediated) that checks that new values added to these enums
correspond.  And there is not even a comment that the values of the
libxl enum and the libxc enum need to be kept in step.


I am not a fan of the cast as a solution.  I would rather, prefer to
regularise the situation.  If my co-maintainers agree about the
desirability of expecting libxl callers to use enum values from Xen
public headers, then I would want to:

 * Change the hypervisor interface
 * Abolish the libxc and libxl enums
 * Provide a compatibility layer in libxl for users of the old
   enum value names and the old type names (do we need to keep
   the old enum in the IDL or does our API stability guarantee apply
   to the generated C bindings?)

Ian.