[PATCH] tools/libxc: Introduce typesafe and side-effect safe min()/max() macros

[PATCH] tools/libxc: Introduce typesafe and side-effect safe min()/max() macros

[Andrew Cooper]

Replace the current users, and remove scattered re-definitions.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxc/xc_core_x86.c                 |    4 ----
 tools/libxc/xc_dom_decompress_unsafe_xz.c |    6 ------
 tools/libxc/xc_domain_restore.c           |    2 +-
 tools/libxc/xc_private.h                  |   12 ++++++++++++
 tools/libxc/xg_private.h                  |    2 +-
 tools/libxc/xg_save_restore.h             |    7 -------
 6 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index e328dcf..f05060a 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -24,10 +24,6 @@
 
 #define GET_FIELD(_p, _f) ((dinfo->guest_width==8) ? ((_p)->x64._f) : ((_p)->x32._f))
 
-#ifndef MAX
-#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
-#endif
-
 int
 xc_core_arch_gpfn_may_present(struct xc_core_arch_context *arch_ctxt,
                               unsigned long pfn)
diff --git a/tools/libxc/xc_dom_decompress_unsafe_xz.c b/tools/libxc/xc_dom_decompress_unsafe_xz.c
index 2a32d40..d4b8ac8 100644
--- a/tools/libxc/xc_dom_decompress_unsafe_xz.c
+++ b/tools/libxc/xc_dom_decompress_unsafe_xz.c
@@ -34,12 +34,6 @@ static inline u32 le32_to_cpup(const u32 *p)
 	return cpu_to_le32(*p);
 }
 
-#define min(x,y) ({ \
-        const typeof(x) _x = (x);       \
-        const typeof(y) _y = (y);       \
-        (void) (&_x == &_y);            \
-        _x < _y ? _x : _y; })
-
 #define min_t(type,x,y) \
         ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
 #define max_t(type,x,y) \
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 071ab6a..1a99370 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -339,7 +339,7 @@ static xen_pfn_t *load_p2m_frame_list(
             /* Any remaining bytes of this chunk: read and discard. */
             while ( chunk_bytes )
             {
-                unsigned long sz = MIN(chunk_bytes, sizeof(xen_pfn_t));
+                unsigned long sz = min((size_t)chunk_bytes, sizeof(xen_pfn_t));
                 if ( RDEXACT(io_fd, &p2m_fl_zero, sz) )
                 {
                     PERROR("read-and-discard extended-info chunk bytes failed");
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 6cc0f2b..d7afc95 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -367,4 +367,16 @@ int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
 void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
                           uint32_t *port);
 
+#define min(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x < _y) ? _x : _y; })
+
+#define max(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x > _y) ? _x : _y; })
+
 #endif /* __XC_PRIVATE_H__ */
diff --git a/tools/libxc/xg_private.h b/tools/libxc/xg_private.h
index e593364..36651ca 100644
--- a/tools/libxc/xg_private.h
+++ b/tools/libxc/xg_private.h
@@ -159,7 +159,7 @@ static inline xen_pfn_t xc_pfn_to_mfn(xen_pfn_t pfn, xen_pfn_t *p2m,
 /* Size in bytes of the pfn_to_mfn_frame_list     */
 #define P2M_GUEST_FL_SIZE ((P2M_FL_ENTRIES) * (dinfo->guest_width))
 #define P2M_TOOLS_FL_SIZE ((P2M_FL_ENTRIES) *                           \
-                           MAX((sizeof (xen_pfn_t)), dinfo->guest_width))
+                           max(sizeof(xen_pfn_t), (size_t)dinfo->guest_width))
 
 /* Masks for PTE<->PFN conversions */
 #define MADDR_BITS_X86  ((dinfo->guest_width == 8) ? 52 : 44)
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index aa93c13..bdd9009 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -392,10 +392,3 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
     else                                                           \
         memset(&(_p)->x32._f[0], (_v), sizeof((_p)->x32._f));      \
 } while (0)
-
-#ifndef MAX
-#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
-#endif
-#ifndef MIN
-#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
-#endif
-- 
1.7.10.4

Re: [PATCH] tools/libxc: Introduce typesafe and side-effect safe min()/max() macros

[Ian Campbell]

On Tue, 2014-07-08 at 14:21 +0100, Andrew Cooper wrote:
> Replace the current users, and remove scattered re-definitions.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

> -                unsigned long sz = MIN(chunk_bytes, sizeof(xen_pfn_t));
> +                unsigned long sz = min((size_t)chunk_bytes, sizeof(xen_pfn_t));

FWIW on the hyperevisor side we have max_t and min_t to handle these
cases where a cast is needed.

I think it's a little bit safer too since rather than casting it assigns
the arguments to temporary variables of the named types (so any type
promotion etc mismatches are flagged rather than hidden by the cast).

Ian.

Re: [PATCH] tools/libxc: Introduce typesafe and side-effect safe min()/max() macros

[Ian Campbell]

On Wed, 2014-07-09 at 16:54 +0100, Ian Campbell wrote:
> On Tue, 2014-07-08 at 14:21 +0100, Andrew Cooper wrote:
> > Replace the current users, and remove scattered re-definitions.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> 
> > -                unsigned long sz = MIN(chunk_bytes, sizeof(xen_pfn_t));
> > +                unsigned long sz = min((size_t)chunk_bytes, sizeof(xen_pfn_t));
> 
> FWIW on the hyperevisor side we have max_t and min_t to handle these
> cases where a cast is needed.

Which I've just noticed/remembered I promoted to xc_private.h in my ARM
p2m superpages series!

> I think it's a little bit safer too since rather than casting it assigns
> the arguments to temporary variables of the named types (so any type
> promotion etc mismatches are flagged rather than hidden by the cast).
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] tools/libxc: Introduce typesafe and side-effect safe min()/max() macros

[Andrew Cooper]

On 10/07/14 10:35, Ian Campbell wrote:
> On Wed, 2014-07-09 at 16:54 +0100, Ian Campbell wrote:
>> On Tue, 2014-07-08 at 14:21 +0100, Andrew Cooper wrote:
>>> Replace the current users, and remove scattered re-definitions.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
>>
>>> -                unsigned long sz = MIN(chunk_bytes, sizeof(xen_pfn_t));
>>> +                unsigned long sz = min((size_t)chunk_bytes, sizeof(xen_pfn_t));
>> FWIW on the hyperevisor side we have max_t and min_t to handle these
>> cases where a cast is needed.
> Which I've just noticed/remembered I promoted to xc_private.h in my ARM
> p2m superpages series!

I noticed that.  I guess it depends which patch gets committed first.

This patch is a prerequisite of the writev() patch, which is a
prerequisite for my migration v2.

If you wish, I can do a v2 which moves min_t/max_t at the same time and
uses them in preference.

~Andrew

Re: [PATCH] tools/libxc: Introduce typesafe and side-effect safe min()/max() macros

[Ian Campbell]

On Thu, 2014-07-10 at 10:41 +0100, Andrew Cooper wrote:
> On 10/07/14 10:35, Ian Campbell wrote:
> > On Wed, 2014-07-09 at 16:54 +0100, Ian Campbell wrote:
> >> On Tue, 2014-07-08 at 14:21 +0100, Andrew Cooper wrote:
> >>> Replace the current users, and remove scattered re-definitions.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> >>
> >>> -                unsigned long sz = MIN(chunk_bytes, sizeof(xen_pfn_t));
> >>> +                unsigned long sz = min((size_t)chunk_bytes, sizeof(xen_pfn_t));
> >> FWIW on the hyperevisor side we have max_t and min_t to handle these
> >> cases where a cast is needed.
> > Which I've just noticed/remembered I promoted to xc_private.h in my ARM
> > p2m superpages series!
> 
> I noticed that.  I guess it depends which patch gets committed first.

I've just committed a batch of patches which includes my min_t/max_t
one.

> This patch is a prerequisite of the writev() patch, which is a
> prerequisite for my migration v2.
> 
> If you wish, I can do a v2 which moves min_t/max_t at the same time and
> uses them in preference.

If you rebase onto staging  should get min_t and max_t provided already,
then a v2 which uses them as appropriate would be appreciated, thanks.

Ian.