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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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?

---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(
+    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;

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