[PATCH v2 0/6] Misc Improvements

[PATCH v2 0/6] Misc Improvements

[Andrew Cooper]

Misc improvements for libxc, which are prerequisites for the migration v2
series, but not directly related to it.  Some of these patches have been
posted to the list singally before;  Their submission here superceeds any
previous submission.

Andrew Cooper (5):
  tools/libxc: Annotate xc_report_error with __attribute__((format))
  tools/libxc: Annotate xc_osdep_log with __attribute__((format))
  tools/libxc: Use _Static_assert if available
  tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples
  tools/libxc: Add Valgrind client requests

David Vrabel (1):
  tools/libxc: add DECLARE_HYPERCALL_BUFFER_SHADOW()

 config/Tools.mk.in                 |    1 +
 tools/configure.ac                 |    2 ++
 tools/libxc/Makefile               |    4 ++++
 tools/libxc/xc_core.c              |    2 +-
 tools/libxc/xc_dom_arm.c           |    2 +-
 tools/libxc/xc_dom_x86.c           |    4 ++--
 tools/libxc/xc_domain_restore.c    |    6 +++---
 tools/libxc/xc_gnttab.c            |    2 +-
 tools/libxc/xc_minios.c            |    2 +-
 tools/libxc/xc_private.h           |   18 +++++++++++++++++-
 tools/libxc/xenctrl.h              |   18 ++++++++++++++++++
 tools/libxc/xenctrl_osdep_ENOSYS.c |   32 ++++++++++++++++----------------
 tools/libxc/xenctrlosdep.h         |    3 ++-
 tools/misc/xen-hptool.c            |    2 --
 tools/misc/xen-mfndump.c           |    2 --
 tools/xenpaging/xenpaging.c        |    2 +-
 16 files changed, 70 insertions(+), 32 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/6] tools/libxc: Annotate xc_report_error with __attribute__((format))

[Andrew Cooper]

This helps the compiler spot printf formatting errors.

Fix up all errors discovered.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---
v2: Fix up %zd for ssize_t and %PRIx32 for QEMU Magic
---
 tools/libxc/xc_core.c           |    2 +-
 tools/libxc/xc_domain_restore.c |    6 +++---
 tools/libxc/xc_gnttab.c         |    2 +-
 tools/libxc/xc_private.h        |    3 ++-
 tools/xenpaging/xenpaging.c     |    2 +-
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/xc_core.c b/tools/libxc/xc_core.c
index 4bc1abb..dfa424b 100644
--- a/tools/libxc/xc_core.c
+++ b/tools/libxc/xc_core.c
@@ -497,7 +497,7 @@ xc_domain_dumpcore_via_callback(xc_interface *xch,
     ctxt = calloc(sizeof(*ctxt), info.max_vcpu_id + 1);
     if ( !ctxt )
     {
-        PERROR("Could not allocate vcpu context array", domid);
+        PERROR("Could not allocate vcpu context array");
         goto out;
     }
 
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 89af1ad..b80f867 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -102,7 +102,7 @@ static ssize_t rdexact(xc_interface *xch, struct restore_ctx *ctx,
             errno = 0;
         }
         if ( len <= 0 ) {
-            ERROR("%s failed (read rc: %d, errno: %d)", __func__, len, errno);
+            ERROR("%s failed (read rc: %zd, errno: %d)", __func__, len, errno);
             return -1;
         }
         offset += len;
@@ -443,7 +443,7 @@ static int compat_buffer_qemu(xc_interface *xch, struct restore_ctx *ctx,
     }
 
     if ( memcmp(qbuf, "QEVM", 4) ) {
-        ERROR("Invalid QEMU magic: 0x%08x", *(unsigned long*)qbuf);
+        ERROR("Invalid QEMU magic: 0x%08"PRIx32, *(uint32_t*)qbuf);
         free(qbuf);
         return -1;
     }
@@ -1802,7 +1802,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
     } else if (pagebuf.acpi_ioport_location == 0) {
         DBGPRINTF("Use old firmware ioport from the checkpoint\n");
     } else {
-        ERROR("Error, unknow acpi ioport location (%i)", pagebuf.acpi_ioport_location);
+        ERROR("Error, unknow acpi ioport location (%"PRId64")", pagebuf.acpi_ioport_location);
     }
 
     tdatatmp = tdata;
diff --git a/tools/libxc/xc_gnttab.c b/tools/libxc/xc_gnttab.c
index 3b6058c..4076e47 100644
--- a/tools/libxc/xc_gnttab.c
+++ b/tools/libxc/xc_gnttab.c
@@ -75,7 +75,7 @@ static void *_gnttab_map_table(xc_interface *xch, int domid, int *gnt_num)
 
     if ( rc || (query.status != GNTST_okay) )
     {
-        ERROR("Could not query dom's grant size\n", domid);
+        ERROR("Could not query dom%d's grant size\n", domid);
         return NULL;
     }
 
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 670a82d..29e926c 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -93,7 +93,8 @@ struct xc_interface_core {
     xc_osdep_handle  ops_handle; /* opaque data for xc_osdep_ops */
 };
 
-void xc_report_error(xc_interface *xch, int code, const char *fmt, ...);
+void xc_report_error(xc_interface *xch, int code, const char *fmt, ...)
+    __attribute__((format(printf,3,4)));
 void xc_reportv(xc_interface *xch, xentoollog_logger *lg, xentoollog_level,
                 int code, const char *fmt, va_list args)
      __attribute__((format(printf,5,0)));
diff --git a/tools/xenpaging/xenpaging.c b/tools/xenpaging/xenpaging.c
index 5ef2f09..82c1ee4 100644
--- a/tools/xenpaging/xenpaging.c
+++ b/tools/xenpaging/xenpaging.c
@@ -912,7 +912,7 @@ int main(int argc, char *argv[])
 
             if ( req.gfn > paging->max_pages )
             {
-                ERROR("Requested gfn %"PRIx64" higher than max_pages %lx\n", req.gfn, paging->max_pages);
+                ERROR("Requested gfn %"PRIx64" higher than max_pages %x\n", req.gfn, paging->max_pages);
                 goto out;
             }
 
-- 
1.7.10.4

[PATCH v2 2/6] tools/libxc: Annotate xc_osdep_log with __attribute__((format))

[Andrew Cooper]

This helps the compiler spot printf formatting errors.

Fix up resulting errors in xenctrl_osdep_ENOSYS.c.  Substitute %p for the
slightly less bad %lx when trying to format an opaque structure.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---
v2: %p -> %lx
---
 tools/libxc/xenctrl_osdep_ENOSYS.c |   32 ++++++++++++++++----------------
 tools/libxc/xenctrlosdep.h         |    3 ++-
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/tools/libxc/xenctrl_osdep_ENOSYS.c b/tools/libxc/xenctrl_osdep_ENOSYS.c
index 4821342..abdf3d5 100644
--- a/tools/libxc/xenctrl_osdep_ENOSYS.c
+++ b/tools/libxc/xenctrl_osdep_ENOSYS.c
@@ -15,19 +15,19 @@
 
 static xc_osdep_handle ENOSYS_privcmd_open(xc_interface *xch)
 {
-    IPRINTF(xch, "ENOSYS_privcmd: opening handle %p\n", (void *)1);
+    IPRINTF(xch, "ENOSYS_privcmd: opening handle %d\n", 1);
     return (xc_osdep_handle)1; /*dummy*/
 }
 
 static int ENOSYS_privcmd_close(xc_interface *xch, xc_osdep_handle h)
 {
-    IPRINTF(xch, "ENOSYS_privcmd: closing handle %p\n", h);
+    IPRINTF(xch, "ENOSYS_privcmd: closing handle %lx\n", h);
     return 0;
 }
 
 static int ENOSYS_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcmd_hypercall_t *hypercall)
 {
-    IPRINTF(xch, "ENOSYS_privcmd %p: hypercall: %02lld(%#llx,%#llx,%#llx,%#llx,%#llx)\n",
+    IPRINTF(xch, "ENOSYS_privcmd %lx: hypercall: %02lld(%#llx,%#llx,%#llx,%#llx,%#llx)\n",
             h, hypercall->op,
             hypercall->arg[0], hypercall->arg[1], hypercall->arg[2],
             hypercall->arg[3], hypercall->arg[4]);
@@ -37,21 +37,21 @@ static int ENOSYS_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcm
 static void *ENOSYS_privcmd_map_foreign_batch(xc_interface *xch, xc_osdep_handle h, uint32_t dom, int prot,
                                       xen_pfn_t *arr, int num)
 {
-    IPRINTF(xch, "ENOSYS_privcmd %p: map_foreign_batch: dom%d prot %#x arr %p num %d\n", h, dom, prot, arr, num);
+    IPRINTF(xch, "ENOSYS_privcmd %lx: map_foreign_batch: dom%d prot %#x arr %p num %d\n", h, dom, prot, arr, num);
     return MAP_FAILED;
 }
 
 static void *ENOSYS_privcmd_map_foreign_bulk(xc_interface *xch, xc_osdep_handle h, uint32_t dom, int prot,
                                      const xen_pfn_t *arr, int *err, unsigned int num)
 {
-    IPRINTF(xch, "ENOSYS_privcmd %p: map_foreign_buld: dom%d prot %#x arr %p err %p num %d\n", h, dom, prot, arr, err, num);
+    IPRINTF(xch, "ENOSYS_privcmd %lx map_foreign_buld: dom%d prot %#x arr %p err %p num %d\n", h, dom, prot, arr, err, num);
     return MAP_FAILED;
 }
 
 static void *ENOSYS_privcmd_map_foreign_range(xc_interface *xch, xc_osdep_handle h, uint32_t dom, int size, int prot,
                                       unsigned long mfn)
 {
-    IPRINTF(xch, "ENOSYS_privcmd %p: map_foreign_range: dom%d size %#x prot %#x mfn %ld\n", h, dom, size, prot, mfn);
+    IPRINTF(xch, "ENOSYS_privcmd %lx map_foreign_range: dom%d size %#x prot %#x mfn %ld\n", h, dom, size, prot, mfn);
     return MAP_FAILED;
 }
 
@@ -59,7 +59,7 @@ static void *ENOSYS_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handl
                                        size_t chunksize, privcmd_mmap_entry_t entries[],
                                        int nentries)
 {
-    IPRINTF(xch, "ENOSYS_privcmd %p: map_foreign_ranges: dom%d size %zd prot %#x chunksize %zd entries %p num %d\n", h, dom, size, prot, chunksize, entries, nentries);
+    IPRINTF(xch, "ENOSYS_privcmd %lx map_foreign_ranges: dom%d size %zd prot %#x chunksize %zd entries %p num %d\n", h, dom, size, prot, chunksize, entries, nentries);
     return MAP_FAILED;
 }
 
@@ -85,59 +85,59 @@ static xc_osdep_handle ENOSYS_evtchn_open(xc_interface *xce)
 
 static int ENOSYS_evtchn_close(xc_interface *xce, xc_osdep_handle h)
 {
-    IPRINTF(xce, "ENOSYS_evtchn: closing handle %p\n", h);
+    IPRINTF(xce, "ENOSYS_evtchn: closing handle %lx\n", h);
     return 0;
 }
 
 static int ENOSYS_evtchn_fd(xc_interface *xce, xc_osdep_handle h)
 {
-    IPRINTF(xce, "ENOSYS_fd %p: fd\n", h);
+    IPRINTF(xce, "ENOSYS_fd %lx fd\n", h);
     return (int)h;
 }
 
 static int ENOSYS_evtchn_notify(xc_interface *xce, xc_osdep_handle h, evtchn_port_t port)
 {
-    IPRINTF(xce, "ENOSYS_evtchn %p: notify: %d\n", h, port);
+    IPRINTF(xce, "ENOSYS_evtchn %lx notify: %d\n", h, port);
     return -ENOSYS;
 }
 
 static int ENOSYS_evtchn_bind_unbound_port(xc_interface *xce, xc_osdep_handle h, int domid)
 {
-    IPRINTF(xce, "ENOSYS_evtchn %p: bind_unbound_port: dom%d\n", h, domid);
+    IPRINTF(xce, "ENOSYS_evtchn %lx bind_unbound_port: dom%d\n", h, domid);
     return -ENOSYS;
 }
 
 
 static int ENOSYS_evtchn_bind_interdomain(xc_interface *xce, xc_osdep_handle h, int domid, evtchn_port_t remote_port)
 {
-    IPRINTF(xce, "ENOSYS_evtchn %p: bind_interdomain: dmo%d %d\n", h, domid, remote_port);
+    IPRINTF(xce, "ENOSYS_evtchn %lx bind_interdomain: dmo%d %d\n", h, domid, remote_port);
     return -ENOSYS;
 }
 
 
 static int ENOSYS_evtchn_bind_virq(xc_interface *xce, xc_osdep_handle h, unsigned int virq)
 {
-    IPRINTF(xce, "ENOSYS_evtchn %p: bind_virq: %d\n", h, virq);
+    IPRINTF(xce, "ENOSYS_evtchn %lx bind_virq: %d\n", h, virq);
     return -ENOSYS;
 }
 
 
 static int ENOSYS_evtchn_unbind(xc_interface *xce, xc_osdep_handle h, evtchn_port_t port)
 {
-    IPRINTF(xce, "ENOSYS_evtchn %p: unbind: %d\n", h, port);
+    IPRINTF(xce, "ENOSYS_evtchn %lx unbind: %d\n", h, port);
     return -ENOSYS;
 }
 
 
 static evtchn_port_or_error_t ENOSYS_evtchn_pending(xc_interface *xce, xc_osdep_handle h)
 {
-    IPRINTF(xce, "ENOSYS_evtchn %p: pending\n", h);
+    IPRINTF(xce, "ENOSYS_evtchn %lx pending\n", h);
     return -ENOSYS;
 }
 
 static int ENOSYS_evtchn_unmask(xc_interface *xce, xc_osdep_handle h, evtchn_port_t port)
 {
-    IPRINTF(xce, "ENOSYS_evtchn %p: unmask: %d\n", h, port);
+    IPRINTF(xce, "ENOSYS_evtchn %lx unmask: %d\n", h, port);
     return -ENOSYS;
 }
 
diff --git a/tools/libxc/xenctrlosdep.h b/tools/libxc/xenctrlosdep.h
index e610a24..e97944b 100644
--- a/tools/libxc/xenctrlosdep.h
+++ b/tools/libxc/xenctrlosdep.h
@@ -157,7 +157,8 @@ void *xc_map_foreign_bulk_compat(xc_interface *xch, xc_osdep_handle h,
                                  const xen_pfn_t *arr, int *err, unsigned int num);
 
 /* Report errors through xc_interface */
-void xc_osdep_log(xc_interface *xch, xentoollog_level level, int code, const char *fmt, ...);
+void xc_osdep_log(xc_interface *xch, xentoollog_level level, int code,
+                  const char *fmt, ...) __attribute__((format(printf, 4, 5)));
 
 #endif
 
-- 
1.7.10.4

[PATCH v2 3/6] tools/libxc: Use _Static_assert if available

[Andrew Cooper]

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_private.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 29e926c..12476ec 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -48,7 +48,11 @@
 #define PAGE_MASK               XC_PAGE_MASK
 
 /* Force a compilation error if condition is true */
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
+#define XC_BUILD_BUG_ON(p) ({ _Static_assert(!(p), "!(" #p ")"); })
+#else
 #define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { int:-!!(p); }))
+#endif
 
 /*
 ** Define max dirty page cache to permit during save/restore -- need to balance 
-- 
1.7.10.4

[PATCH v2 4/6] tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples

[Andrew Cooper]

The conditional test is needed because xc_private.h is included into minios
for stubdomains, where ARRAY_SIZE is already defined.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

--
v2: Find more areas of the codebase which are poking at libxc private internals
---
 tools/libxc/xc_dom_arm.c |    2 +-
 tools/libxc/xc_dom_x86.c |    4 ++--
 tools/libxc/xc_minios.c  |    2 +-
 tools/libxc/xc_private.h |    4 ++++
 tools/misc/xen-hptool.c  |    2 --
 tools/misc/xen-mfndump.c |    2 --
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 75f8363..cc64363 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -230,7 +230,7 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
 
     domctl.domain = domid;
     domctl.cmd    = XEN_DOMCTL_set_address_size;
-    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
+    for ( i = 0; i < ARRAY_SIZE(types); i++ )
         if ( !strcmp(types[i].guest, guest_type) )
             domctl.u.address_size.size = types[i].size;
     if ( domctl.u.address_size.size == 0 )
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e034d62..bf06fe4 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -716,7 +716,7 @@ static int x86_compat(xc_interface *xch, domid_t domid, char *guest_type)
     memset(&domctl, 0, sizeof(domctl));
     domctl.domain = domid;
     domctl.cmd    = XEN_DOMCTL_set_address_size;
-    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
+    for ( i = 0; i < ARRAY_SIZE(types); i++ )
         if ( !strcmp(types[i].guest, guest_type) )
             domctl.u.address_size.size = types[i].size;
     if ( domctl.u.address_size.size == 0 )
@@ -887,7 +887,7 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
     xen_pfn_t shinfo;
     int i, rc;
 
-    for ( i = 0; i < sizeof(types) / sizeof(types[0]); i++ )
+    for ( i = 0; i < ARRAY_SIZE(types); i++ )
         if ( !strcmp(types[i].guest, dom->guest_type) )
             pgd_type = types[i].pgd_type;
 
diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
index e621417..e703684 100644
--- a/tools/libxc/xc_minios.c
+++ b/tools/libxc/xc_minios.c
@@ -87,7 +87,7 @@ static int minios_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcm
     int i, ret;
 
     call.op = hypercall->op;
-    for (i = 0; i < sizeof(hypercall->arg) / sizeof(*hypercall->arg); i++)
+    for (i = 0; i < ARRAY_SIZE(hypercall->arg); i++)
 	call.args[i] = hypercall->arg[i];
 
     ret = HYPERVISOR_multicall(&call, 1);
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 12476ec..213f607 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -54,6 +54,10 @@
 #define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { int:-!!(p); }))
 #endif
 
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
+#endif
+
 /*
 ** Define max dirty page cache to permit during save/restore -- need to balance 
 ** keeping cache usage down with CPU impact of invalidating too often.
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index d0e8e90..a16213a 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -4,8 +4,6 @@
 #include <xenstore.h>
 #include <unistd.h>
 
-#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
-
 static xc_interface *xch;
 
 void show_help(void)
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 1af3bd8..33e4b91 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -6,8 +6,6 @@
 
 #include "xg_save_restore.h"
 
-#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
-
 static xc_interface *xch;
 
 int help_func(int argc, char *argv[])
-- 
1.7.10.4

Re: [PATCH v2 4/6] tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples

[Ian Campbell]

On Mon, 2014-06-09 at 16:41 +0100, Andrew Cooper wrote:
> The conditional test is needed because xc_private.h is included into minios
> for stubdomains, where ARRAY_SIZE is already defined.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> --
> v2: Find more areas of the codebase which are poking at libxc private internals
> ---
>  tools/libxc/xc_dom_arm.c |    2 +-
>  tools/libxc/xc_dom_x86.c |    4 ++--
>  tools/libxc/xc_minios.c  |    2 +-
>  tools/libxc/xc_private.h |    4 ++++
>  tools/misc/xen-hptool.c  |    2 --
>  tools/misc/xen-mfndump.c |    2 --

Not tools/misc/xenpm.c too?

tools/misc/* really didn't ought to be including xc_private.h I think.

Rather than make this worse how about defining XC_ARRAY_SIZE in the
public API? Or if not perhaps
   #undef ARRAY_SIZE /* We shouldn't be including xc_private.h */
in those so that an eventual removal of xc_private from them isn't
blocked? (I think I slightly prefer the second option)

minios really shouldn't be polluting the namespace like that either.
Perhaps that's a yack Ian J is going to cover as part of the rump kernel
stuff...

>  6 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 75f8363..cc64363 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -230,7 +230,7 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
>  
>      domctl.domain = domid;
>      domctl.cmd    = XEN_DOMCTL_set_address_size;
> -    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>          if ( !strcmp(types[i].guest, guest_type) )
>              domctl.u.address_size.size = types[i].size;
>      if ( domctl.u.address_size.size == 0 )
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index e034d62..bf06fe4 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -716,7 +716,7 @@ static int x86_compat(xc_interface *xch, domid_t domid, char *guest_type)
>      memset(&domctl, 0, sizeof(domctl));
>      domctl.domain = domid;
>      domctl.cmd    = XEN_DOMCTL_set_address_size;
> -    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>          if ( !strcmp(types[i].guest, guest_type) )
>              domctl.u.address_size.size = types[i].size;
>      if ( domctl.u.address_size.size == 0 )
> @@ -887,7 +887,7 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
>      xen_pfn_t shinfo;
>      int i, rc;
>  
> -    for ( i = 0; i < sizeof(types) / sizeof(types[0]); i++ )
> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>          if ( !strcmp(types[i].guest, dom->guest_type) )
>              pgd_type = types[i].pgd_type;
>  
> diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
> index e621417..e703684 100644
> --- a/tools/libxc/xc_minios.c
> +++ b/tools/libxc/xc_minios.c
> @@ -87,7 +87,7 @@ static int minios_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcm
>      int i, ret;
>  
>      call.op = hypercall->op;
> -    for (i = 0; i < sizeof(hypercall->arg) / sizeof(*hypercall->arg); i++)
> +    for (i = 0; i < ARRAY_SIZE(hypercall->arg); i++)
>  	call.args[i] = hypercall->arg[i];
>  
>      ret = HYPERVISOR_multicall(&call, 1);
> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
> index 12476ec..213f607 100644
> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
> @@ -54,6 +54,10 @@
>  #define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { int:-!!(p); }))
>  #endif
>  
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> +#endif
> +
>  /*
>  ** Define max dirty page cache to permit during save/restore -- need to balance 
>  ** keeping cache usage down with CPU impact of invalidating too often.
> diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
> index d0e8e90..a16213a 100644
> --- a/tools/misc/xen-hptool.c
> +++ b/tools/misc/xen-hptool.c
> @@ -4,8 +4,6 @@
>  #include <xenstore.h>
>  #include <unistd.h>
>  
> -#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
> -
>  static xc_interface *xch;
>  
>  void show_help(void)
> diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
> index 1af3bd8..33e4b91 100644
> --- a/tools/misc/xen-mfndump.c
> +++ b/tools/misc/xen-mfndump.c
> @@ -6,8 +6,6 @@
>  
>  #include "xg_save_restore.h"
>  
> -#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
> -
>  static xc_interface *xch;
>  
>  int help_func(int argc, char *argv[])

Re: [PATCH v2 4/6] tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples

[Andrew Cooper]

On 10/06/14 13:40, Ian Campbell wrote:
> On Mon, 2014-06-09 at 16:41 +0100, Andrew Cooper wrote:
>> The conditional test is needed because xc_private.h is included into minios
>> for stubdomains, where ARRAY_SIZE is already defined.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>
>> --
>> v2: Find more areas of the codebase which are poking at libxc private internals
>> ---
>>  tools/libxc/xc_dom_arm.c |    2 +-
>>  tools/libxc/xc_dom_x86.c |    4 ++--
>>  tools/libxc/xc_minios.c  |    2 +-
>>  tools/libxc/xc_private.h |    4 ++++
>>  tools/misc/xen-hptool.c  |    2 --
>>  tools/misc/xen-mfndump.c |    2 --
> Not tools/misc/xenpm.c too?

No - xenpm.c is well behaved and doesn't use private headers.  (and I
discovered this by ripping them all out and xenpm failed to compile)

>
> tools/misc/* really didn't ought to be including xc_private.h I think.

I agree, but it didn't look trivial to fix.

>
> Rather than make this worse how about defining XC_ARRAY_SIZE in the
> public API?

I don't think this should really live in the public API.

>  Or if not perhaps
>    #undef ARRAY_SIZE /* We shouldn't be including xc_private.h */
> in those so that an eventual removal of xc_private from them isn't
> blocked? (I think I slightly prefer the second option)

This is my preferred option.

~Andrew

>
> minios really shouldn't be polluting the namespace like that either.
> Perhaps that's a yack Ian J is going to cover as part of the rump kernel
> stuff...
>
>>  6 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index 75f8363..cc64363 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -230,7 +230,7 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
>>  
>>      domctl.domain = domid;
>>      domctl.cmd    = XEN_DOMCTL_set_address_size;
>> -    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
>> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>>          if ( !strcmp(types[i].guest, guest_type) )
>>              domctl.u.address_size.size = types[i].size;
>>      if ( domctl.u.address_size.size == 0 )
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index e034d62..bf06fe4 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -716,7 +716,7 @@ static int x86_compat(xc_interface *xch, domid_t domid, char *guest_type)
>>      memset(&domctl, 0, sizeof(domctl));
>>      domctl.domain = domid;
>>      domctl.cmd    = XEN_DOMCTL_set_address_size;
>> -    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
>> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>>          if ( !strcmp(types[i].guest, guest_type) )
>>              domctl.u.address_size.size = types[i].size;
>>      if ( domctl.u.address_size.size == 0 )
>> @@ -887,7 +887,7 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
>>      xen_pfn_t shinfo;
>>      int i, rc;
>>  
>> -    for ( i = 0; i < sizeof(types) / sizeof(types[0]); i++ )
>> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>>          if ( !strcmp(types[i].guest, dom->guest_type) )
>>              pgd_type = types[i].pgd_type;
>>  
>> diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
>> index e621417..e703684 100644
>> --- a/tools/libxc/xc_minios.c
>> +++ b/tools/libxc/xc_minios.c
>> @@ -87,7 +87,7 @@ static int minios_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcm
>>      int i, ret;
>>  
>>      call.op = hypercall->op;
>> -    for (i = 0; i < sizeof(hypercall->arg) / sizeof(*hypercall->arg); i++)
>> +    for (i = 0; i < ARRAY_SIZE(hypercall->arg); i++)
>>  	call.args[i] = hypercall->arg[i];
>>  
>>      ret = HYPERVISOR_multicall(&call, 1);
>> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
>> index 12476ec..213f607 100644
>> --- a/tools/libxc/xc_private.h
>> +++ b/tools/libxc/xc_private.h
>> @@ -54,6 +54,10 @@
>>  #define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { int:-!!(p); }))
>>  #endif
>>  
>> +#ifndef ARRAY_SIZE
>> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>> +#endif
>> +
>>  /*
>>  ** Define max dirty page cache to permit during save/restore -- need to balance 
>>  ** keeping cache usage down with CPU impact of invalidating too often.
>> diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
>> index d0e8e90..a16213a 100644
>> --- a/tools/misc/xen-hptool.c
>> +++ b/tools/misc/xen-hptool.c
>> @@ -4,8 +4,6 @@
>>  #include <xenstore.h>
>>  #include <unistd.h>
>>  
>> -#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
>> -
>>  static xc_interface *xch;
>>  
>>  void show_help(void)
>> diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
>> index 1af3bd8..33e4b91 100644
>> --- a/tools/misc/xen-mfndump.c
>> +++ b/tools/misc/xen-mfndump.c
>> @@ -6,8 +6,6 @@
>>  
>>  #include "xg_save_restore.h"
>>  
>> -#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
>> -
>>  static xc_interface *xch;
>>  
>>  int help_func(int argc, char *argv[])
>

Re: [PATCH v2 4/6] tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples

[Ian Campbell]

On Tue, 2014-06-10 at 13:53 +0100, Andrew Cooper wrote:
> On 10/06/14 13:40, Ian Campbell wrote:
> > On Mon, 2014-06-09 at 16:41 +0100, Andrew Cooper wrote:
> >> The conditional test is needed because xc_private.h is included into minios
> >> for stubdomains, where ARRAY_SIZE is already defined.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> CC: Ian Campbell <Ian.Campbell@citrix.com>
> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >>
> >> --
> >> v2: Find more areas of the codebase which are poking at libxc private internals
> >> ---
> >>  tools/libxc/xc_dom_arm.c |    2 +-
> >>  tools/libxc/xc_dom_x86.c |    4 ++--
> >>  tools/libxc/xc_minios.c  |    2 +-
> >>  tools/libxc/xc_private.h |    4 ++++
> >>  tools/misc/xen-hptool.c  |    2 --
> >>  tools/misc/xen-mfndump.c |    2 --
> > Not tools/misc/xenpm.c too?
> 
> No - xenpm.c is well behaved and doesn't use private headers.  (and I
> discovered this by ripping them all out and xenpm failed to compile)

Aha!

> >  Or if not perhaps
> >    #undef ARRAY_SIZE /* We shouldn't be including xc_private.h */
> > in those so that an eventual removal of xc_private from them isn't
> > blocked? (I think I slightly prefer the second option)
> 
> This is my preferred option.

Works for me then, thanks.

(FYI if you are planning to I have queued patch 1-3 and 5 for applying)

Ian.

[PATCH v3 4/6] tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples

[Andrew Cooper]

xen-hptool and xen-mfndump include xc_private.h.  This is bad, but not trivial
to fix, so they gain a protective #undef and a stern comment.

MiniOS leaks ARRAY_SIZE into the libxc namespace as part of a stubdom build.
Therefore, xc_private.h gains an #ifndef until MiniOS is fixed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

--
v2: Find more areas of the codebase which are poking at libxc private internals
v3: Refine how xen-{hptool,mfndump} deal with the namespace issue
---
 tools/libxc/xc_dom_arm.c |    2 +-
 tools/libxc/xc_dom_x86.c |    4 ++--
 tools/libxc/xc_minios.c  |    2 +-
 tools/libxc/xc_private.h |    5 +++++
 tools/misc/xen-hptool.c  |    1 +
 tools/misc/xen-mfndump.c |    1 +
 6 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 75f8363..cc64363 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -230,7 +230,7 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
 
     domctl.domain = domid;
     domctl.cmd    = XEN_DOMCTL_set_address_size;
-    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
+    for ( i = 0; i < ARRAY_SIZE(types); i++ )
         if ( !strcmp(types[i].guest, guest_type) )
             domctl.u.address_size.size = types[i].size;
     if ( domctl.u.address_size.size == 0 )
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e034d62..bf06fe4 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -716,7 +716,7 @@ static int x86_compat(xc_interface *xch, domid_t domid, char *guest_type)
     memset(&domctl, 0, sizeof(domctl));
     domctl.domain = domid;
     domctl.cmd    = XEN_DOMCTL_set_address_size;
-    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
+    for ( i = 0; i < ARRAY_SIZE(types); i++ )
         if ( !strcmp(types[i].guest, guest_type) )
             domctl.u.address_size.size = types[i].size;
     if ( domctl.u.address_size.size == 0 )
@@ -887,7 +887,7 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
     xen_pfn_t shinfo;
     int i, rc;
 
-    for ( i = 0; i < sizeof(types) / sizeof(types[0]); i++ )
+    for ( i = 0; i < ARRAY_SIZE(types); i++ )
         if ( !strcmp(types[i].guest, dom->guest_type) )
             pgd_type = types[i].pgd_type;
 
diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
index e621417..e703684 100644
--- a/tools/libxc/xc_minios.c
+++ b/tools/libxc/xc_minios.c
@@ -87,7 +87,7 @@ static int minios_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcm
     int i, ret;
 
     call.op = hypercall->op;
-    for (i = 0; i < sizeof(hypercall->arg) / sizeof(*hypercall->arg); i++)
+    for (i = 0; i < ARRAY_SIZE(hypercall->arg); i++)
 	call.args[i] = hypercall->arg[i];
 
     ret = HYPERVISOR_multicall(&call, 1);
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 12476ec..4447cec 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -54,6 +54,11 @@
 #define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { int:-!!(p); }))
 #endif
 
+#ifndef ARRAY_SIZE /* MiniOS leaks ARRAY_SIZE into our namespace as part of a
+                    * stubdom build.  It shouldn't... */
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
+#endif
+
 /*
 ** Define max dirty page cache to permit during save/restore -- need to balance 
 ** keeping cache usage down with CPU impact of invalidating too often.
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index d0e8e90..1134603 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -4,6 +4,7 @@
 #include <xenstore.h>
 #include <unistd.h>
 
+#undef ARRAY_SIZE /* We shouldn't be including xc_private.h */
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
 
 static xc_interface *xch;
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 1af3bd8..0761f6e 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -6,6 +6,7 @@
 
 #include "xg_save_restore.h"
 
+#undef ARRAY_SIZE /* We shouldn't be including xc_private.h */
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
 
 static xc_interface *xch;
-- 
1.7.10.4

Re: [PATCH v3 4/6] tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples

[Ian Campbell]

On Tue, 2014-06-10 at 15:07 +0100, Andrew Cooper wrote:
> xen-hptool and xen-mfndump include xc_private.h.  This is bad, but not trivial
> to fix, so they gain a protective #undef and a stern comment.
> 
> MiniOS leaks ARRAY_SIZE into the libxc namespace as part of a stubdom build.
> Therefore, xc_private.h gains an #ifndef until MiniOS is fixed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked + Applied.

> 
> --

Triple "-" please, or it doesn't have the desired affect.

> v2: Find more areas of the codebase which are poking at libxc private internals
> v3: Refine how xen-{hptool,mfndump} deal with the namespace issue

[PATCH v2 5/6] tools/libxc: add DECLARE_HYPERCALL_BUFFER_SHADOW()

[Andrew Cooper]

From: David Vrabel <david.vrabel@citrix.com>

DECLARE_HYPERCALL_BUFFER_SHADOW() is like DECLARE_HYPERCALL_BUFFER()
except it is backed by an already allocated hypercall buffer.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xenctrl.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 400f0df..b55d857 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -266,6 +266,24 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
     }
 
 /*
+ * Like DECLARE_HYPERCALL_BUFFER() but using an already allocated
+ * hypercall buffer, _hbuf.
+ *
+ * Useful when a hypercall buffer is passed to a function and access
+ * via the user pointer is required.
+ *
+ * See DECLARE_HYPERCALL_BUFFER_ARGUMENT() if the user pointer is not
+ * required.
+ */
+#define DECLARE_HYPERCALL_BUFFER_SHADOW(_type, _name, _hbuf)   \
+    _type *_name = _hbuf->hbuf;                                \
+    xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(_name) = { \
+        .hbuf = (void *)-1,                                    \
+        .param_shadow = _hbuf,                                 \
+        HYPERCALL_BUFFER_INIT_NO_BOUNCE                        \
+    }
+
+/*
  * Declare the necessary data structure to allow a hypercall buffer
  * passed as an argument to a function to be used in the normal way.
  */
-- 
1.7.10.4

[PATCH v2 6/6] tools/libxc: Add Valgrind client requests

[Andrew Cooper]

Valgrind client requests can be used by code to provide extra debugging
information about memory ranges, or to request checks at specific points.

Reference:
  http://valgrind.org/docs/manual/mc-manual.html#mc-manual.clientreqs

Client requests are safe to compile into code for running outside of
valgrind.  Therefore, enable client requests whenever autoconf can find
memcheck.h and debugging builds are enabled.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

Please rerun autogen as part of committing this

v2: Apply some autoconf
---
 config/Tools.mk.in       |    1 +
 tools/configure.ac       |    2 ++
 tools/libxc/Makefile     |    4 ++++
 tools/libxc/xc_private.h |    7 +++++++
 4 files changed, 14 insertions(+)

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 84b2612..5ce6ee1 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -7,6 +7,7 @@ LIBDIR              := $(libdir)
 
 # A debug build of tools?
 debug               := @debug@
+HAVE_MEMCHECK_H     := @HAVE_MEMCHECK_H@
 
 # Tools path
 BISON               := @BISON@
diff --git a/tools/configure.ac b/tools/configure.ac
index 25d7ca3..452cfd5 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -246,6 +246,8 @@ esac
 
 # Checks for header files.
 AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h])
+AC_CHECK_HEADER([valgrind/memcheck.h], [HAVE_MEMCHECK_H="y"])
+AC_SUBST(HAVE_MEMCHECK_H)
 
 AC_OUTPUT()
 
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index a74b19e..3d55757 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -95,6 +95,10 @@ CFLAGS-$(CONFIG_Linux) += -D_GNU_SOURCE
 
 CFLAGS	+= $(PTHREAD_CFLAGS)
 
+ifeq ($(HAVE_MEMCHECK_H)$(debug),yy)
+CFLAGS	+= -DVALGRIND
+endif
+
 CTRL_LIB_OBJS := $(patsubst %.c,%.o,$(CTRL_SRCS-y))
 CTRL_PIC_OBJS := $(patsubst %.c,%.opic,$(CTRL_SRCS-y))
 
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 213f607..52d8c96 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -34,6 +34,13 @@
 
 #include <xen/sys/privcmd.h>
 
+#ifdef VALGRIND
+/* Compile in Valgrind client requests? */
+#include <valgrind/memcheck.h>
+#else
+#define VALGRIND_MAKE_MEM_UNDEFINED(addr, len) /* addr, len */
+#endif
+
 #define DECLARE_HYPERCALL privcmd_hypercall_t hypercall
 #define DECLARE_DOMCTL struct xen_domctl domctl
 #define DECLARE_SYSCTL struct xen_sysctl sysctl
-- 
1.7.10.4

Re: [PATCH v2 6/6] tools/libxc: Add Valgrind client requests

[Ian Campbell]

On Mon, 2014-06-09 at 16:41 +0100, Andrew Cooper wrote:
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 25d7ca3..452cfd5 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -246,6 +246,8 @@ esac
>  
>  # Checks for header files.
>  AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h])
> +AC_CHECK_HEADER([valgrind/memcheck.h], [HAVE_MEMCHECK_H="y"])
> +AC_SUBST(HAVE_MEMCHECK_H)

If you use AC_CHECK_HEADERS (just extending the list right above) then
autoconf will give you HAVE_VALGRIND_MEMCHECK_H automatically, including
in config.h I think.

>  
>  AC_OUTPUT()
>  
> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> index a74b19e..3d55757 100644
> --- a/tools/libxc/Makefile
> +++ b/tools/libxc/Makefile
> @@ -95,6 +95,10 @@ CFLAGS-$(CONFIG_Linux) += -D_GNU_SOURCE
>  
>  CFLAGS	+= $(PTHREAD_CFLAGS)
>  
> +ifeq ($(HAVE_MEMCHECK_H)$(debug),yy)
> +CFLAGS	+= -DVALGRIND
> +endif

Which means that this can move become part of the conditional in
xc_private.h using NDEBUG and HAVE_VALGRIND_MEMCHECK_H.

> +
>  CTRL_LIB_OBJS := $(patsubst %.c,%.o,$(CTRL_SRCS-y))
>  CTRL_PIC_OBJS := $(patsubst %.c,%.opic,$(CTRL_SRCS-y))
>  
> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
> index 213f607..52d8c96 100644
> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
> @@ -34,6 +34,13 @@
>  
>  #include <xen/sys/privcmd.h>
>  
> +#ifdef VALGRIND
> +/* Compile in Valgrind client requests? */
> +#include <valgrind/memcheck.h>
> +#else
> +#define VALGRIND_MAKE_MEM_UNDEFINED(addr, len) /* addr, len */
> +#endif
> +
>  #define DECLARE_HYPERCALL privcmd_hypercall_t hypercall
>  #define DECLARE_DOMCTL struct xen_domctl domctl
>  #define DECLARE_SYSCTL struct xen_sysctl sysctl

[PATCH v3 6/6] tools/libxc: Add Valgrind client requests

[Andrew Cooper]

Valgrind client requests can be used by code to provide extra debugging
information about memory ranges, or to request checks at specific points.

Reference:
  http://valgrind.org/docs/manual/mc-manual.html#mc-manual.clientreqs

Client requests are safe to compile into code for running outside of
valgrind.  Therefore, enable client requests whenever autoconf can find
memcheck.h and debug builds are enabled.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

Please rerun autogen as part of committing this

v2: Apply some autoconf
v3: Rework autoconf interaction
---
 tools/configure.ac       |    2 +-
 tools/libxc/Makefile     |    3 +++
 tools/libxc/xc_private.h |    7 +++++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/configure.ac b/tools/configure.ac
index 25d7ca3..89f1ac7 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -245,7 +245,7 @@ AC_CHECK_LIB([fdt], [fdt_create], [], [AC_MSG_ERROR([Could not find libfdt])])
 esac
 
 # Checks for header files.
-AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h])
+AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h valgrind/memcheck.h])
 
 AC_OUTPUT()
 
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index a74b19e..215101d 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -104,6 +104,9 @@ GUEST_PIC_OBJS := $(patsubst %.c,%.opic,$(GUEST_SRCS-y))
 OSDEP_LIB_OBJS := $(patsubst %.c,%.o,$(OSDEP_SRCS-y))
 OSDEP_PIC_OBJS := $(patsubst %.c,%.opic,$(OSDEP_SRCS-y))
 
+$(CTRL_LIB_OBJS) $(GUEST_LIB_OBJS) $(OSDEP_LIB_OBJS) \
+$(CTRL_PIC_OBJS) $(GUEST_PIC_OBJS) $(OSDEP_PIC_OBJS) : CFLAGS += -include $(XEN_ROOT)/tools/config.h
+
 LIB := libxenctrl.a
 ifneq ($(stubdom),y)
 LIB += libxenctrl.so libxenctrl.so.$(MAJOR) libxenctrl.so.$(MAJOR).$(MINOR)
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 4447cec..c7730f2 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -34,6 +34,13 @@
 
 #include <xen/sys/privcmd.h>
 
+#if defined(HAVE_VALGRIND_MEMCHECK_H) && !defined(NDEBUG)
+/* Compile in Valgrind client requests? */
+#include <valgrind/memcheck.h>
+#else
+#define VALGRIND_MAKE_MEM_UNDEFINED(addr, len) /* addr, len */
+#endif
+
 #define DECLARE_HYPERCALL privcmd_hypercall_t hypercall
 #define DECLARE_DOMCTL struct xen_domctl domctl
 #define DECLARE_SYSCTL struct xen_sysctl sysctl
-- 
1.7.10.4

Re: [PATCH v3 6/6] tools/libxc: Add Valgrind client requests

[Ian Campbell]

On Tue, 2014-06-10 at 15:41 +0100, Andrew Cooper wrote:
> Valgrind client requests can be used by code to provide extra debugging
> information about memory ranges, or to request checks at specific points.
> 
> Reference:
>   http://valgrind.org/docs/manual/mc-manual.html#mc-manual.clientreqs
> 
> Client requests are safe to compile into code for running outside of
> valgrind.  Therefore, enable client requests whenever autoconf can find
> memcheck.h and debug builds are enabled.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked + applied.

I'm assuming you have check for unused variable warnings in
non-debug/non-valgrind configurations. I suppose it is unlikely that
those two parameters would be used only for this check.

> ---
> 
> Please rerun autogen as part of committing this

Done.

Re: [PATCH v2 0/6] Misc Improvements

[Ian Campbell]

On Mon, 2014-06-09 at 16:41 +0100, Andrew Cooper wrote:
> Misc improvements for libxc, which are prerequisites for the migration v2
> series, but not directly related to it.  Some of these patches have been
> posted to the list singally before;  Their submission here superceeds any
> previous submission.
> 
> Andrew Cooper (5):
>   tools/libxc: Annotate xc_report_error with __attribute__((format))
>   tools/libxc: Annotate xc_osdep_log with __attribute__((format))
>   tools/libxc: Use _Static_assert if available

Acked + Applied.

>   tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples
>   tools/libxc: Add Valgrind client requests

Had comments on these.

> David Vrabel (1):
>   tools/libxc: add DECLARE_HYPERCALL_BUFFER_SHADOW()

Applied.

Cheers,
Ian.