| Message ID | 1453183100-50700-1-git-send-email-czylin@uwaterloo.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-19 at 00:58 -0500, Chester Lin wrote: > 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. > I see... > --- a/tools/libxl/libxl_psr.c > +++ b/tools/libxl/libxl_psr.c > @@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, > uint32_t domid, > libxl_for_each_set_bit(socketid, *target_map) { > if (socketid >= nr_sockets) > break; > - if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, > socketid, cbm)) { > + r = xc_psr_cat_set_domain_data(ctx->xch, domid, > (xc_psr_cat_type) type, > + socketid, cbm); > + if (r) { > Is the cast in the function call better than a local variable of xc_psr_cat_type initialized with 'type'? Or would Coverity keep complaining in such a case? If yes to either of the questions, this patch is: Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> Regards, Dario
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.
On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote: > 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, libxl_shutdown_reason has the same issues, libxl_tsc_mode also might, as might libxl_timer_mode. I'm not sure if there are others. I think we generally handle all these the way psr is handled today (with casts and/or explicit conversion switches). I think part of the problem is that it is hard to expose just the desired bits through to the user of libxl without also exposing the full xen hypercall interface (the vast majority of which would be inappropriate to expose to them since libxl is suppose to encapsulate such things). If we can find a way to allow this without that downside and while preserving API compat that would be ok. > 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?) I think we do need to keep the compat, yes, the fact that bit of the C API is autogenerated makes no difference to the end user. The compat API is only needed for callers who define LIBXL_API_VERSION to some older version though (I think, do check the libxl.h comments in case I'm misremembering what we say). As a possible alternative, we could make it such that the IDL generator knows about the linkage and enforces the use of the same values, and automatically provides conversion helpers (essential a cast wrapped in some syntax) for _internal_ use. Ian.
On Tue, 2016-01-19 at 14:21 +0000, Ian Campbell wrote: > On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote: > > 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, > > libxl_shutdown_reason has the same issues, libxl_tsc_mode also might, > as > might libxl_timer_mode. I'm not sure if there are others. > (FTR) libxl_scheduler too, probably? > I think we > generally handle all these the way psr is handled today (with casts > and/or > explicit conversion switches). > and in fact, libxl_scheduler --touched in another patch of this series-- is also handled in a similar way, and in that case, you seemed to be fine with it? (and I'm not complaining, I'm just genuinely confused :-) ) Dario
On 19/01/16 14:21, Ian Campbell wrote: > On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote: >> 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, <snip> > I think part of the problem is that it is hard to expose just the desired > bits through to the user of libxl without also exposing the full xen > hypercall interface (the vast majority of which would be inappropriate to > expose to them since libxl is suppose to encapsulate such things). I agree with this sentiment... > As a possible alternative, we could make it such that the IDL generator > knows about the linkage and enforces the use of the same values, and > automatically provides conversion helpers (essential a cast wrapped in some > syntax) for _internal_ use. ...and using the IDL to enforce or generate values was my first thought. -George
On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote: > 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) Forgot to say in my other reply, but we could try and abolish at least the xc one and have libxl internally use the domctl values. I should also have said I think all of this is a bit much to ask Chester to tackle given that the intro[0] explains that the Coverity stuff is just in order to gain some familiarity before embarking on a "proper" project. Ian. [0] http://lists.xen.org/archives/html/xen-devel/2015-12/msg00629.html
Dario Faggioli writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> On Tue, 2016-01-19 at 14:21 +0000, Ian Campbell wrote:
> > libxl_shutdown_reason has the same issues, libxl_tsc_mode also might,
> > as might libxl_timer_mode. I'm not sure if there are others.
> >
> (FTR) libxl_scheduler too, probably?
Quite possibly. Hrm.
I see that for shutdown_reason Ian C did this deliberately in "libxl:
Remove xen/sched.h from public interface".
I am really not a fan of this approach. I don't remember what I said
about that at the time. The result is multiple enums with the same
values in that need to be kept in step.
However, I'm not sure I can persuade everyone to agree on an
alternative. At the very least we should document the rules in
CODING_STYLE.
Ian.
Ian Campbell writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> > * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
> > * enum xc_psr_cat_type (xenctrl.h)
> > * Enumeration("psr_cbm_type",...) (libxl_types.idl)
>
> Forgot to say in my other reply, but we could try and abolish at least the
> xc one and have libxl internally use the domctl values.
Yes.
I like George's IDL suggestion.
> I should also have said I think all of this is a bit much to ask Chester to
> tackle given that the intro[0] explains that the Coverity stuff is just in
> order to gain some familiarity before embarking on a "proper" project.
Well, quite! Sorry to Chester for having suddenly revealed this
apparently-simple bug to be a swamp.
Ian.
On Tue, Jan 19, 2016 at 2:35 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Ian Campbell writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"): >> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote: >> > * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h) >> > * enum xc_psr_cat_type (xenctrl.h) >> > * Enumeration("psr_cbm_type",...) (libxl_types.idl) >> >> Forgot to say in my other reply, but we could try and abolish at least the >> xc one and have libxl internally use the domctl values. > > Yes. > > I like George's IDL suggestion. Out of curiosity, did anything ever come of this? If not it seems like we should write it down somewhere. -George
On Thu, 2017-01-12 at 18:08 +0000, George Dunlap wrote: > On Tue, Jan 19, 2016 at 2:35 PM, Ian Jackson > <Ian.Jackson@eu.citrix.com> wrote: > > > > > On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote: > > > > > > > > * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h) > > > > * enum xc_psr_cat_type (xenctrl.h) > > > > * Enumeration("psr_cbm_type",...) (libxl_types.idl) > > > > > > Forgot to say in my other reply, but we could try and abolish at > > > least the > > > xc one and have libxl internally use the domctl values. > > > > Yes. > > > > I like George's IDL suggestion. > > Out of curiosity, did anything ever come of this? > AFAICT, no. The patch with the casts was not taken, and we still have all the three enums/types in Xen, libxc and libxl. > If not it seems > like we should write it down somewhere. > Indeed, especially because it was a good idea. Dario
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c index 3d0dc61..1677f9c 100644 --- a/tools/libxl/libxl_psr.c +++ b/tools/libxl/libxl_psr.c @@ -298,7 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, uint64_t cbm) { GC_INIT(ctx); - int rc; + int rc, r; int socketid, nr_sockets; rc = libxl__count_physical_sockets(gc, &nr_sockets); @@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, libxl_for_each_set_bit(socketid, *target_map) { if (socketid >= nr_sockets) break; - if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) { + r = xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type) type, + socketid, cbm); + if (r) { libxl__psr_cat_log_err_msg(gc, errno); rc = ERROR_FAIL; } @@ -326,11 +328,14 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid, uint64_t *cbm_r) { GC_INIT(ctx); - int rc = 0; - - if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r)) { + int rc, r; + r = xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type) type, + target, cbm_r); + if (r) { libxl__psr_cat_log_err_msg(gc, errno); rc = ERROR_FAIL; + } else { + rc = 0; } GC_FREE;
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. No functional changes. Signed-off-by: Chester Lin <czylin@uwaterloo.ca> --- Changed commit message to say that the enums are identical --- tools/libxl/libxl_psr.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)