Re: [Xen-devel] [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > 
> > > Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > 
> > Nacked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > 
> > > > -        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;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel