| Message ID | 20160428204911.GA14217@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 28, 2016 at 09:49:11PM +0100, Wei Liu wrote: > On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote: > > Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"): > > > On Tue, Apr 26, 2016 at 04:52:21PM +0200, Roger Pau Monne wrote: > > > > The xc_psr_* functions expect the type to be xc_psr_cat_type instead of > > > > libxl_psr_cbm_type, so do the conversion. > > > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > > Acked-by: Wei Liu <wei.liu2@citrix.com> > > > > Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > > > > > - if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, > > cbm)) { > > > > + if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_typ > > e)type, > > > > + socketid, cbm)) { > > > > I'm very much against introducing casts which are not absolutely > > necessary. Casts are a big hammer which can suppress important > > warnings (such as conversions between integers and pointers). > > > > This anomaly with the same enum defined in two places with two names > > is pretty poor. But if we are to perpetuate it, as perhaps we must, > > then rather than casting at each conversion point, we should introduce > > an inline function which contains the cast. That way each call site > > remains more typesafe. > > > > The two enums aren't going away any time soon. > > Does the following diff meet your requirement? Hello, Thanks for doing this. > ---8<--- > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c > index 40f2d5f..7a34c04 100644 > --- a/tools/libxl/libxl_psr.c > +++ b/tools/libxl/libxl_psr.c > @@ -293,12 +293,18 @@ out: > return rc; > } > > +static inline xc_psr_cat_type libxl_psr_cbm_type_to_libxc_psr_cat_type( ^ extra _ needed. > + libxl_psr_cbm_type type) > +{ > + BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type)); > + return (xc_psr_cat_type)type; In order to prevent using a cast, we could use a union: union { libxl_psr_cbm_type libxl_psr; xc_psr_cat_type xc_psr; } u; u.libxl_psr = type; return u.xc_psr; > +} > + > int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, > libxl_psr_cbm_type type, libxl_bitmap *target_map, > uint64_t cbm) > { > GC_INIT(ctx); > - BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type)); > int rc; > int socketid, nr_sockets; > > @@ -309,9 +315,13 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, > } > > libxl_for_each_set_bit(socketid, *target_map) { > + xc_psr_cat_type xc_type; > + > if (socketid >= nr_sockets) > break; > - if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type)type, > + > + xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type); > + if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type, > socketid, cbm)) { > libxl__psr_cat_log_err_msg(gc, errno); > rc = ERROR_FAIL; > @@ -329,8 +339,9 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid, > { > GC_INIT(ctx); > int rc = 0; > + xc_psr_cat_type xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type); > > - if (xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)type, > + if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type, > target, cbm_r)) { > libxl__psr_cat_log_err_msg(gc, errno); > rc = ERROR_FAIL;
Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote:
> > I'm very much against introducing casts which are not absolutely
> > necessary. Casts are a big hammer which can suppress important
> > warnings (such as conversions between integers and pointers).
> >
> > This anomaly with the same enum defined in two places with two names
> > is pretty poor. But if we are to perpetuate it, as perhaps we must,
> > then rather than casting at each conversion point, we should introduce
> > an inline function which contains the cast. That way each call site
> > remains more typesafe.
>
> The two enums aren't going away any time soon.
Sadly, I think you're right.
> Does the following diff meet your requirement?
Yes, that is exactly the kind of thing I was thinking of. It makes
the cast non-routine by putting it in a dedicated function whose
authors/readers know to check it's right.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(with a suitable commit message)
Roger Pau Monne writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> On Thu, Apr 28, 2016 at 09:49:11PM +0100, Wei Liu wrote:
> > + BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
> > + return (xc_psr_cat_type)type;
>
> In order to prevent using a cast, we could use a union:
>
> union {
> libxl_psr_cbm_type libxl_psr;
> xc_psr_cat_type xc_psr;
> } u;
>
> u.libxl_psr = type;
> return u.xc_psr;
No, not really.
Firstly, although we compile with -fno-strict-aliasing, this is a bad
example in these days of hostile optimisers: without
-fno-strict-aliasing, the compiler is allowed to assume that the loads
and stores are to different variables, even though the contrary is
evident.
Secondly, it's clumsy.
Thirdly, this kind of union aliasing is normally used for
representation (structure) punning.
Thanks,
Ian.
On Wed, May 18, 2016 at 03:45:22PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"): > > On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote: > > > I'm very much against introducing casts which are not absolutely > > > necessary. Casts are a big hammer which can suppress important > > > warnings (such as conversions between integers and pointers). > > > > > > This anomaly with the same enum defined in two places with two names > > > is pretty poor. But if we are to perpetuate it, as perhaps we must, > > > then rather than casting at each conversion point, we should introduce > > > an inline function which contains the cast. That way each call site > > > remains more typesafe. > > > > The two enums aren't going away any time soon. > > Sadly, I think you're right. > > > Does the following diff meet your requirement? > > Yes, that is exactly the kind of thing I was thinking of. It makes > the cast non-routine by putting it in a dedicated function whose > authors/readers know to check it's right. > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > (with a suitable commit message) Thanks. I will submit a proper patch with your ack. Wei.
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c index 40f2d5f..7a34c04 100644 --- a/tools/libxl/libxl_psr.c +++ b/tools/libxl/libxl_psr.c @@ -293,12 +293,18 @@ out: return rc; } +static inline xc_psr_cat_type libxl_psr_cbm_type_to_libxc_psr_cat_type( + libxl_psr_cbm_type type) +{ + BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type)); + return (xc_psr_cat_type)type; +} + int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, libxl_psr_cbm_type type, libxl_bitmap *target_map, uint64_t cbm) { GC_INIT(ctx); - BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type)); int rc; int socketid, nr_sockets; @@ -309,9 +315,13 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, } libxl_for_each_set_bit(socketid, *target_map) { + xc_psr_cat_type xc_type; + if (socketid >= nr_sockets) break; - if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type)type, + + xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type); + if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type, socketid, cbm)) { libxl__psr_cat_log_err_msg(gc, errno); rc = ERROR_FAIL; @@ -329,8 +339,9 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid, { GC_INIT(ctx); int rc = 0; + xc_psr_cat_type xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type); - if (xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)type, + if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type, target, cbm_r)) { libxl__psr_cat_log_err_msg(gc, errno); rc = ERROR_FAIL;