[PATCH 0 of 2] libxc: Print domain ID with libxc save/restore messages

[PATCH 0 of 2] libxc: Print domain ID with libxc save/restore messages

[George Dunlap]

XenServer implements logging functions to redirect libxc output to
syslog.  Unfortunately, this means that when running a large number of
operations in parallel, the status messages from different save and
restore operations get mixed up, and it's hard to tell which is which.

This patch series adds code to print the domain ID in the messages of
all save-restore code.  

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

[PATCH 1 of 2] libxc: Pass the save_ctx struct to more functions

[George Dunlap]

This is in preparation for a patch that will print the domain ID
in the error messages.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -125,7 +125,8 @@ static int noncached_write(xc_interface 
     return rc;
 }
 
-static int outbuf_init(xc_interface *xch, struct outbuf* ob, size_t size)
+static int outbuf_init(xc_interface *xch, struct save_ctx *ctx,
+                       struct outbuf* ob, size_t size)
 {
     memset(ob, 0, sizeof(*ob));
 
@@ -139,7 +140,7 @@ static int outbuf_init(xc_interface *xch
     return 0;
 }
 
-static inline int outbuf_write(xc_interface *xch,
+static inline int outbuf_write(xc_interface *xch, struct save_ctx *ctx,
                                struct outbuf* ob, void* buf, size_t len)
 {
     if ( len > ob->size - ob->pos ) {
@@ -154,7 +155,8 @@ static inline int outbuf_write(xc_interf
 }
 
 /* prep for nonblocking I/O */
-static int outbuf_flush(xc_interface *xch, struct outbuf* ob, int fd)
+static int outbuf_flush(xc_interface *xch, struct save_ctx *ctx,
+                        struct outbuf* ob, int fd)
 {
     int rc;
     int cur = 0;
@@ -180,45 +182,46 @@ static int outbuf_flush(xc_interface *xc
 }
 
 /* if there's no room in the buffer, flush it and try again. */
-static inline int outbuf_hardwrite(xc_interface *xch,
+static inline int outbuf_hardwrite(xc_interface *xch, struct save_ctx *ctx,
                                    struct outbuf* ob, int fd, void* buf,
                                    size_t len)
 {
     if ( !len )
         return 0;
 
-    if ( !outbuf_write(xch, ob, buf, len) )
+    if ( !outbuf_write(xch, ctx, ob, buf, len) )
         return 0;
 
-    if ( outbuf_flush(xch, ob, fd) < 0 )
+    if ( outbuf_flush(xch, ctx, ob, fd) < 0 )
         return -1;
 
-    return outbuf_write(xch, ob, buf, len);
+    return outbuf_write(xch, ctx, ob, buf, len);
 }
 
 /* start buffering output once we've reached checkpoint mode. */
-static inline int write_buffer(xc_interface *xch,
+static inline int write_buffer(xc_interface *xch, struct save_ctx *ctx,
                                int dobuf, struct outbuf* ob, int fd, void* buf,
                                size_t len)
 {
     if ( dobuf )
-        return outbuf_hardwrite(xch, ob, fd, buf, len);
+        return outbuf_hardwrite(xch, ctx, ob, fd, buf, len);
     else
         return write_exact(fd, buf, len);
 }
 
 /* like write_buffer for noncached, which returns number of bytes written */
-static inline int write_uncached(xc_interface *xch,
+static inline int write_uncached(xc_interface *xch, struct save_ctx *ctx,
                                    int dobuf, struct outbuf* ob, int fd,
                                    void* buf, size_t len)
 {
     if ( dobuf )
-        return outbuf_hardwrite(xch, ob, fd, buf, len) ? -1 : len;
+        return outbuf_hardwrite(xch, ctx, ob, fd, buf, len) ? -1 : len;
     else
         return noncached_write(xch, ob, fd, buf, len);
 }
 
-static int write_compressed(xc_interface *xch, comp_ctx *compress_ctx,
+static int write_compressed(xc_interface *xch, struct save_ctx *ctx,
+                            comp_ctx *compress_ctx,
                             int dobuf, struct outbuf* ob, int fd)
 {
     int rc = 0;
@@ -231,7 +234,7 @@ static int write_compressed(xc_interface
         /* check for available space (atleast 8k) */
         if ((ob->pos + header + XC_PAGE_SIZE * 2) > ob->size)
         {
-            if (outbuf_flush(xch, ob, fd) < 0)
+            if (outbuf_flush(xch, ctx, ob, fd) < 0)
             {
                 ERROR("Error when flushing outbuf intermediate");
                 return -1;
@@ -245,20 +248,20 @@ static int write_compressed(xc_interface
         if (!rc)
             return 0;
 
-        if (outbuf_hardwrite(xch, ob, fd, &marker, sizeof(marker)) < 0)
+        if (outbuf_hardwrite(xch, ctx, ob, fd, &marker, sizeof(marker)) < 0)
         {
             PERROR("Error when writing marker (errno %d)", errno);
             return -1;
         }
 
-        if (outbuf_hardwrite(xch, ob, fd, &compbuf_len, sizeof(compbuf_len)) < 0)
+        if (outbuf_hardwrite(xch, ctx, ob, fd, &compbuf_len, sizeof(compbuf_len)) < 0)
         {
             PERROR("Error when writing compbuf_len (errno %d)", errno);
             return -1;
         }
 
         ob->pos += (size_t) compbuf_len;
-        if (!dobuf && outbuf_flush(xch, ob, fd) < 0)
+        if (!dobuf && outbuf_flush(xch, ctx, ob, fd) < 0)
         {
             ERROR("Error when writing compressed chunk");
             return -1;
@@ -273,7 +276,8 @@ struct time_stats {
     long long d0_cpu, d1_cpu;
 };
 
-static int print_stats(xc_interface *xch, uint32_t domid, int pages_sent,
+static int print_stats(xc_interface *xch, struct save_ctx *ctx,
+                       uint32_t domid, int pages_sent,
                        struct time_stats *last,
                        xc_shadow_op_stats_t *stats, int print)
 {
@@ -349,7 +353,8 @@ static int analysis_phase(xc_interface *
 }
 
 static int suspend_and_state(int (*suspend)(void*), void* data,
-                             xc_interface *xch, int io_fd, int dom,
+                             xc_interface *xch, struct save_ctx *ctx,
+                             int io_fd, int dom,
                              xc_dominfo_t *info)
 {
     if ( !(*suspend)(data) )
@@ -904,7 +909,7 @@ int xc_domain_save(xc_interface *xch, in
         return 1;
     }
 
-    outbuf_init(xch, &ob_pagebuf, OUTBUF_SIZE);
+    outbuf_init(xch, ctx, &ob_pagebuf, OUTBUF_SIZE);
 
     memset(ctx, 0, sizeof(*ctx));
 
@@ -984,7 +989,7 @@ int xc_domain_save(xc_interface *xch, in
     else
     {
         /* This is a non-live suspend. Suspend the domain .*/
-        if ( suspend_and_state(callbacks->suspend, callbacks->data, xch,
+        if ( suspend_and_state(callbacks->suspend, callbacks->data, xch, ctx,
                                io_fd, dom, &info) )
         {
             ERROR("Domain appears not to have suspended");
@@ -999,7 +1004,7 @@ int xc_domain_save(xc_interface *xch, in
             ERROR("Failed to create compression context");
             goto out;
         }
-        outbuf_init(xch, &ob_tailbuf, OUTBUF_SIZE/4);
+        outbuf_init(xch, ctx, &ob_tailbuf, OUTBUF_SIZE/4);
     }
 
     last_iter = !live;
@@ -1094,7 +1099,7 @@ int xc_domain_save(xc_interface *xch, in
         DPRINTF("Had %d unexplained entries in p2m table\n", err);
     }
 
-    print_stats(xch, dom, 0, &time_stats, &shadow_stats, 0);
+    print_stats(xch, ctx, dom, 0, &time_stats, &shadow_stats, 0);
 
     tmem_saved = xc_tmem_save(xch, dom, io_fd, live, XC_SAVE_ID_TMEM);
     if ( tmem_saved == -1 )
@@ -1110,9 +1115,9 @@ int xc_domain_save(xc_interface *xch, in
     }
 
   copypages:
-#define wrexact(fd, buf, len) write_buffer(xch, last_iter, ob, (fd), (buf), (len))
-#define wruncached(fd, live, buf, len) write_uncached(xch, last_iter, ob, (fd), (buf), (len))
-#define wrcompressed(fd) write_compressed(xch, compress_ctx, last_iter, ob, (fd))
+#define wrexact(fd, buf, len) write_buffer(xch, ctx, last_iter, ob, (fd), (buf), (len))
+#define wruncached(fd, live, buf, len) write_uncached(xch, ctx, last_iter, ob, (fd), (buf), (len))
+#define wrcompressed(fd) write_compressed(xch, ctx, compress_ctx, last_iter, ob, (fd))
 
     ob = &ob_pagebuf; /* Holds pfn_types, pages/compressed pages */
     /* Now write out each data page, canonicalising page tables as we go... */
@@ -1494,7 +1499,7 @@ int xc_domain_save(xc_interface *xch, in
 
         if ( last_iter )
         {
-            print_stats( xch, dom, sent_this_iter, &time_stats, &shadow_stats, 1);
+            print_stats( xch, ctx, dom, sent_this_iter, &time_stats, &shadow_stats, 1);
 
             DPRINTF("Total pages sent= %ld (%.2fx)\n",
                     total_sent, ((float)total_sent)/dinfo->p2m_size );
@@ -1531,7 +1536,7 @@ int xc_domain_save(xc_interface *xch, in
                 last_iter = 1;
 
                 if ( suspend_and_state(callbacks->suspend, callbacks->data,
-                                       xch, io_fd, dom, &info) )
+                                       xch, ctx, io_fd, dom, &info) )
                 {
                     ERROR("Domain appears not to have suspended");
                     goto out;
@@ -1564,7 +1569,7 @@ int xc_domain_save(xc_interface *xch, in
 
             sent_last_iter = sent_this_iter;
 
-            print_stats(xch, dom, sent_this_iter, &time_stats, &shadow_stats, 1);
+            print_stats(xch, ctx, dom, sent_this_iter, &time_stats, &shadow_stats, 1);
 
         }
     } /* end of infinite for loop */
@@ -2023,7 +2028,7 @@ int xc_domain_save(xc_interface *xch, in
     }
 
     /* Flush last write and discard cache for file. */
-    if ( outbuf_flush(xch, ob, io_fd) < 0 ) {
+    if ( outbuf_flush(xch, ctx, ob, io_fd) < 0 ) {
         PERROR("Error when flushing output buffer");
         rc = 1;
     }
@@ -2038,18 +2043,18 @@ int xc_domain_save(xc_interface *xch, in
         callbacks->checkpoint(callbacks->data) > 0)
     {
         /* reset stats timer */
-        print_stats(xch, dom, 0, &time_stats, &shadow_stats, 0);
+        print_stats(xch, ctx, dom, 0, &time_stats, &shadow_stats, 0);
 
         rc = 1;
         /* last_iter = 1; */
-        if ( suspend_and_state(callbacks->suspend, callbacks->data, xch,
+        if ( suspend_and_state(callbacks->suspend, callbacks->data, xch, ctx,
                                io_fd, dom, &info) )
         {
             ERROR("Domain appears not to have suspended");
             goto out;
         }
         DPRINTF("SUSPEND shinfo %08lx\n", info.shared_info_frame);
-        print_stats(xch, dom, 0, &time_stats, &shadow_stats, 1);
+        print_stats(xch, ctx, dom, 0, &time_stats, &shadow_stats, 1);
 
         if ( xc_shadow_control(xch, dom,
                                XEN_DOMCTL_SHADOW_OP_CLEAN, HYPERCALL_BUFFER(to_send),

Re: [PATCH 1 of 2] libxc: Pass the save_ctx struct to more functions

[Ian Jackson]

George Dunlap writes ("[Xen-devel] [PATCH 1 of 2] libxc: Pass the save_ctx struct to more functions"):
> This is in preparation for a patch that will print the domain ID
> in the error messages.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 2 of 2] libxc: Print domain ID in save restore messages

[George Dunlap]

XenServer redirects all libxc output to /var/log/messages, so when a
large stress test is running, it's hard to tell which message belongs
to which domain.

This patch adds the domain ID to output made by the save/restore
functions.

To do this, we introduce a layer of indirection in the libxc print
macros, so that they can be "interposed on" by individual functions.

We then add the domain ID to the context structs in the save and restore
paths, and replace the toplevel macros with ones which add the domain to the
output.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -33,7 +33,20 @@
 #include <xen/hvm/ioreq.h>
 #include <xen/hvm/params.h>
 
+/* Override default output f'ns with functions that include the domain id */
+#undef IPRINTF
+#define IPRINTF(_F, _A...) _IPRINTF("d%d:" _F, ctx->dom, ## _A)
+#undef DPRINTF
+#define DPRINTF(_F, _A...) _DPRINTF("d%d:" _F, ctx->dom, ## _A)
+#undef DBGPRINTF
+#define DBGPRINTF(_F, _A...) _DBGPRINTF("d%d:" _F, ctx->dom, ## _A)
+#undef ERROR
+#define ERROR(_F, _A...) _ERROR("d%d:" _F, ctx->dom, ## _A)
+#undef PERROR
+#define PERROR(_F, _A...) _PERROR("d%d:" _F, ctx->dom, ## _A)
+
 struct restore_ctx {
+    int dom;
     unsigned long max_mfn; /* max mfn of the current host machine */
     unsigned long hvirt_start; /* virtual starting address of the hypervisor */
     unsigned int pt_levels; /* #levels of page tables used by the current guest */
@@ -1362,6 +1375,9 @@ int xc_domain_restore(xc_interface *xch,
 
     memset(ctx, 0, sizeof(*ctx));
 
+    /* Tag all errors with domain */
+    ctx->dom=dom;
+
     ctxt = xc_hypercall_buffer_alloc(xch, ctxt, sizeof(*ctxt));
 
     if ( ctxt == NULL )
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -34,6 +34,19 @@
 
 #include <xen/hvm/params.h>
 
+/* Override default output f'ns with functions that include the domain id */
+#undef IPRINTF
+#define IPRINTF(_F, _A...) _IPRINTF("d%d:" _F, ctx->dom, ## _A)
+#undef DPRINTF
+#define DPRINTF(_F, _A...) _DPRINTF("d%d:" _F, ctx->dom, ## _A)
+#undef DBGPRINTF
+#define DBGPRINTF(_F, _A...) _DBGPRINTF("d%d:" _F, ctx->dom, ## _A)
+#undef ERROR
+#define ERROR(_F, _A...) _ERROR("d%d:" _F, ctx->dom, ## _A)
+#undef PERROR
+#define PERROR(_F, _A...) _PERROR("d%d:" _F, ctx->dom, ## _A)
+
+
 /*
 ** Default values for important tuning parameters. Can override by passing
 ** non-zero replacement values to xc_domain_save().
@@ -45,6 +58,7 @@
 #define DEF_MAX_FACTOR   3   /* never send more than 3x p2m_size  */
 
 struct save_ctx {
+    int dom;
     unsigned long hvirt_start; /* virtual starting address of the hypervisor */
     unsigned int pt_levels; /* #levels of page tables used by the current guest */
     unsigned long max_mfn; /* max mfn of the whole machine */
@@ -529,6 +543,7 @@ static int canonicalize_pagetable(struct
     return race;
 }
 
+/* NB Since this is a public function, we can't use the defined print macros */
 xen_pfn_t *xc_map_m2p(xc_interface *xch,
                                  unsigned long max_mfn,
                                  int prot,
@@ -547,20 +562,20 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
     extent_start = calloc(m2p_chunks, sizeof(xen_pfn_t));
     if ( !extent_start )
     {
-        ERROR("failed to allocate space for m2p mfns");
+        _ERROR("failed to allocate space for m2p mfns");
         goto err0;
     }
 
     if ( xc_machphys_mfn_list(xch, m2p_chunks, extent_start) )
     {
-        PERROR("xc_get_m2p_mfns");
+        _PERROR("xc_get_m2p_mfns");
         goto err1;
     }
 
     entries = calloc(m2p_chunks, sizeof(privcmd_mmap_entry_t));
     if (entries == NULL)
     {
-        ERROR("failed to allocate space for mmap entries");
+        _ERROR("failed to allocate space for mmap entries");
         goto err1;
     }
 
@@ -572,7 +587,7 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
 			entries, m2p_chunks);
     if (m2p == NULL)
     {
-        PERROR("xc_mmap_foreign_ranges failed");
+        _PERROR("xc_mmap_foreign_ranges failed");
         goto err2;
     }
 
@@ -913,6 +928,9 @@ int xc_domain_save(xc_interface *xch, in
 
     memset(ctx, 0, sizeof(*ctx));
 
+    /* Tag all errors with domain */
+    ctx->dom=dom;
+
     /* If no explicit control parameters given, use defaults */
     max_iters  = max_iters  ? : DEF_MAX_ITERS;
     max_factor = max_factor ? : DEF_MAX_FACTOR;
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -119,14 +119,21 @@ void xc_report_progress_step(xc_interfac
 
 /* anamorphic macros:  struct xc_interface *xch  must be in scope */
 
-#define IPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_INFO,0, _f , ## _a)
-#define DPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_DETAIL,0, _f , ## _a)
-#define DBGPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_DEBUG,0, _f , ## _a)
+/* Provide these macros, as well as macros that can be #undef'd and over-ridden */
+#define _IPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_INFO,0, _f , ## _a)
+#define _DPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_DETAIL,0, _f , ## _a)
+#define _DBGPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_DEBUG,0, _f , ## _a)
 
-#define ERROR(_m, _a...)  xc_report_error(xch,XC_INTERNAL_ERROR,_m , ## _a )
-#define PERROR(_m, _a...) xc_report_error(xch,XC_INTERNAL_ERROR,_m \
+#define _ERROR(_m, _a...)  xc_report_error(xch,XC_INTERNAL_ERROR,_m , ## _a )
+#define _PERROR(_m, _a...) xc_report_error(xch,XC_INTERNAL_ERROR,_m \
                   " (%d = %s)", ## _a , errno, xc_strerror(xch, errno))
 
+#define IPRINTF(_f, _a...) _IPRINTF(_f, ## _a)
+#define DPRINTF(_f, _a...) _DPRINTF(_f, ## _a)
+#define DBGPRINTF(_f, _a...) _DBGPRINTF(_f, ## _a)
+#define ERROR(_m, _a...) _ERROR(_m, ## _a)
+#define PERROR(_m, _a...) _PERROR(_m, ## _a)
+
 /*
  * HYPERCALL ARGUMENT BUFFERS
  *

Re: [PATCH 2 of 2] libxc: Print domain ID in save restore messages

[Ian Campbell]

On Fri, 2012-03-09 at 12:29 -0500, George Dunlap wrote:
> XenServer redirects all libxc output to /var/log/messages, so when a
> large stress test is running, it's hard to tell which message belongs
> to which domain.
> 
> This patch adds the domain ID to output made by the save/restore
> functions.
> 
> To do this, we introduce a layer of indirection in the libxc print
> macros, so that they can be "interposed on" by individual functions.
> 
> We then add the domain ID to the context structs in the save and restore
> paths, and replace the toplevel macros with ones which add the domain to the
> output.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -33,7 +33,20 @@
>  #include <xen/hvm/ioreq.h>
>  #include <xen/hvm/params.h>
>  
> +/* Override default output f'ns with functions that include the domain id */
> +#undef IPRINTF
> +#define IPRINTF(_F, _A...) _IPRINTF("d%d:" _F, ctx->dom, ## _A)

I think

#define DIPRINTF(_F, _A...) IPRINTF("d%s:" _F, ctx->dom, ## _A)

(or DOMIPRINTF etc) would end up looking nicer than these undef and the
_IPRINTFs you've had to scatter around the place.

Ian.

Re: [PATCH 2 of 2] libxc: Print domain ID in save restore messages

[George Dunlap]

On Fri, 2012-03-09 at 18:19 +0000, Ian Campbell wrote:
> On Fri, 2012-03-09 at 12:29 -0500, George Dunlap wrote:
> > XenServer redirects all libxc output to /var/log/messages, so when a
> > large stress test is running, it's hard to tell which message belongs
> > to which domain.
> > 
> > This patch adds the domain ID to output made by the save/restore
> > functions.
> > 
> > To do this, we introduce a layer of indirection in the libxc print
> > macros, so that they can be "interposed on" by individual functions.
> > 
> > We then add the domain ID to the context structs in the save and restore
> > paths, and replace the toplevel macros with ones which add the domain to the
> > output.
> > 
> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > 
> > diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> > --- a/tools/libxc/xc_domain_restore.c
> > +++ b/tools/libxc/xc_domain_restore.c
> > @@ -33,7 +33,20 @@
> >  #include <xen/hvm/ioreq.h>
> >  #include <xen/hvm/params.h>
> >  
> > +/* Override default output f'ns with functions that include the domain id */
> > +#undef IPRINTF
> > +#define IPRINTF(_F, _A...) _IPRINTF("d%d:" _F, ctx->dom, ## _A)
> 
> I think
> 
> #define DIPRINTF(_F, _A...) IPRINTF("d%s:" _F, ctx->dom, ## _A)
> 
> (or DOMIPRINTF etc) would end up looking nicer than these undef and the
> _IPRINTFs you've had to scatter around the place.

Perhaps; but that also requires that every person changing this file
forever more must remember to write "DIPRINTF" instead of "IPRINTF".
(And it requires me to change 10x as many LoC.)

 -George

Re: [PATCH 2 of 2] libxc: Print domain ID in save restore messages

[Ian Campbell]

On Fri, 2012-03-09 at 13:25 -0500, George Dunlap wrote:
> On Fri, 2012-03-09 at 18:19 +0000, Ian Campbell wrote:
> > On Fri, 2012-03-09 at 12:29 -0500, George Dunlap wrote:
> > > XenServer redirects all libxc output to /var/log/messages, so when a
> > > large stress test is running, it's hard to tell which message belongs
> > > to which domain.
> > > 
> > > This patch adds the domain ID to output made by the save/restore
> > > functions.
> > > 
> > > To do this, we introduce a layer of indirection in the libxc print
> > > macros, so that they can be "interposed on" by individual functions.
> > > 
> > > We then add the domain ID to the context structs in the save and restore
> > > paths, and replace the toplevel macros with ones which add the domain to the
> > > output.
> > > 
> > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > > 
> > > diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> > > --- a/tools/libxc/xc_domain_restore.c
> > > +++ b/tools/libxc/xc_domain_restore.c
> > > @@ -33,7 +33,20 @@
> > >  #include <xen/hvm/ioreq.h>
> > >  #include <xen/hvm/params.h>
> > >  
> > > +/* Override default output f'ns with functions that include the domain id */
> > > +#undef IPRINTF
> > > +#define IPRINTF(_F, _A...) _IPRINTF("d%d:" _F, ctx->dom, ## _A)
> > 
> > I think
> > 
> > #define DIPRINTF(_F, _A...) IPRINTF("d%s:" _F, ctx->dom, ## _A)
> > 
> > (or DOMIPRINTF etc) would end up looking nicer than these undef and the
> > _IPRINTFs you've had to scatter around the place.
> 
> Perhaps; but that also requires that every person changing this file
> forever more must remember to write "DIPRINTF" instead of "IPRINTF".

They already have to remember to write IPRINTF instead of printf(), most
people will just copy whatever is used nearby, whether that is IPRINTF,
_IPRINTF or DIPRINTF...

> (And it requires me to change 10x as many LoC.)

That's irritating but not a show stopper IMHO.

Coming from the other angle can you omit all uses of _IPRINTF by passing
the context around a few more places? I'd have expected that everything
in xc_domain_save.c was ultimately called from xc_domain_save and
therefore the is a dom which could be printed?

Ian.

Re: [PATCH 2 of 2] libxc: Print domain ID in save restore messages

[George Dunlap]

On Fri, 2012-03-09 at 18:33 +0000, Ian Campbell wrote:
> On Fri, 2012-03-09 at 13:25 -0500, George Dunlap wrote:
> > On Fri, 2012-03-09 at 18:19 +0000, Ian Campbell wrote:
> > > On Fri, 2012-03-09 at 12:29 -0500, George Dunlap wrote:
> > > > XenServer redirects all libxc output to /var/log/messages, so when a
> > > > large stress test is running, it's hard to tell which message belongs
> > > > to which domain.
> > > > 
> > > > This patch adds the domain ID to output made by the save/restore
> > > > functions.
> > > > 
> > > > To do this, we introduce a layer of indirection in the libxc print
> > > > macros, so that they can be "interposed on" by individual functions.
> > > > 
> > > > We then add the domain ID to the context structs in the save and restore
> > > > paths, and replace the toplevel macros with ones which add the domain to the
> > > > output.
> > > > 
> > > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > > > 
> > > > diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> > > > --- a/tools/libxc/xc_domain_restore.c
> > > > +++ b/tools/libxc/xc_domain_restore.c
> > > > @@ -33,7 +33,20 @@
> > > >  #include <xen/hvm/ioreq.h>
> > > >  #include <xen/hvm/params.h>
> > > >  
> > > > +/* Override default output f'ns with functions that include the domain id */
> > > > +#undef IPRINTF
> > > > +#define IPRINTF(_F, _A...) _IPRINTF("d%d:" _F, ctx->dom, ## _A)
> > > 
> > > I think
> > > 
> > > #define DIPRINTF(_F, _A...) IPRINTF("d%s:" _F, ctx->dom, ## _A)
> > > 
> > > (or DOMIPRINTF etc) would end up looking nicer than these undef and the
> > > _IPRINTFs you've had to scatter around the place.
> > 
> > Perhaps; but that also requires that every person changing this file
> > forever more must remember to write "DIPRINTF" instead of "IPRINTF".
> 
> They already have to remember to write IPRINTF instead of printf(), most
> people will just copy whatever is used nearby, whether that is IPRINTF,
> _IPRINTF or DIPRINTF...
> 
> > (And it requires me to change 10x as many LoC.)
> 
> That's irritating but not a show stopper IMHO.
> 
> Coming from the other angle can you omit all uses of _IPRINTF by passing
> the context around a few more places? I'd have expected that everything
> in xc_domain_save.c was ultimately called from xc_domain_save and
> therefore the is a dom which could be printed?

There's a non-static function xc_map_m2p() which is defined in
xc_domain_save.c, but called from xc_offline_page.c and
tools/tests/mce-test/tools/xce-mceinj.c.  That should probably then be
moved to another file in any case.

If I move that function to a different file, so that there are no
_IPRINTF's, would that suffice?

 -George

> 
> Ian.
> 

Re: [PATCH 2 of 2] libxc: Print domain ID in save restore messages

[Ian Campbell]

On Mon, 2012-03-12 at 10:37 +0000, George Dunlap wrote:
> On Fri, 2012-03-09 at 18:33 +0000, Ian Campbell wrote:
> > On Fri, 2012-03-09 at 13:25 -0500, George Dunlap wrote:
> > > On Fri, 2012-03-09 at 18:19 +0000, Ian Campbell wrote:
> > > > On Fri, 2012-03-09 at 12:29 -0500, George Dunlap wrote:
> > > > > XenServer redirects all libxc output to /var/log/messages, so when a
> > > > > large stress test is running, it's hard to tell which message belongs
> > > > > to which domain.
> > > > > 
> > > > > This patch adds the domain ID to output made by the save/restore
> > > > > functions.
> > > > > 
> > > > > To do this, we introduce a layer of indirection in the libxc print
> > > > > macros, so that they can be "interposed on" by individual functions.
> > > > > 
> > > > > We then add the domain ID to the context structs in the save and restore
> > > > > paths, and replace the toplevel macros with ones which add the domain to the
> > > > > output.
> > > > > 
> > > > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > > > > 
> > > > > diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> > > > > --- a/tools/libxc/xc_domain_restore.c
> > > > > +++ b/tools/libxc/xc_domain_restore.c
> > > > > @@ -33,7 +33,20 @@
> > > > >  #include <xen/hvm/ioreq.h>
> > > > >  #include <xen/hvm/params.h>
> > > > >  
> > > > > +/* Override default output f'ns with functions that include the domain id */
> > > > > +#undef IPRINTF
> > > > > +#define IPRINTF(_F, _A...) _IPRINTF("d%d:" _F, ctx->dom, ## _A)
> > > > 
> > > > I think
> > > > 
> > > > #define DIPRINTF(_F, _A...) IPRINTF("d%s:" _F, ctx->dom, ## _A)
> > > > 
> > > > (or DOMIPRINTF etc) would end up looking nicer than these undef and the
> > > > _IPRINTFs you've had to scatter around the place.
> > > 
> > > Perhaps; but that also requires that every person changing this file
> > > forever more must remember to write "DIPRINTF" instead of "IPRINTF".
> > 
> > They already have to remember to write IPRINTF instead of printf(), most
> > people will just copy whatever is used nearby, whether that is IPRINTF,
> > _IPRINTF or DIPRINTF...
> > 
> > > (And it requires me to change 10x as many LoC.)
> > 
> > That's irritating but not a show stopper IMHO.
> > 
> > Coming from the other angle can you omit all uses of _IPRINTF by passing
> > the context around a few more places? I'd have expected that everything
> > in xc_domain_save.c was ultimately called from xc_domain_save and
> > therefore the is a dom which could be printed?
> 
> There's a non-static function xc_map_m2p() which is defined in
> xc_domain_save.c, but called from xc_offline_page.c and
> tools/tests/mce-test/tools/xce-mceinj.c.  That should probably then be
> moved to another file in any case.
> 
> If I move that function to a different file, so that there are no
> _IPRINTF's, would that suffice?

I don't much like the undef'ery stuff but I guess it'll do.

Ian.

Re: [PATCH 2 of 2] libxc: Print domain ID in save restore messages

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH 2 of 2] libxc: Print domain ID in save restore messages"):
> On Fri, 2012-03-09 at 18:33 +0000, Ian Campbell wrote:
> > They already have to remember to write IPRINTF instead of printf(), most
> > people will just copy whatever is used nearby, whether that is IPRINTF,
> > _IPRINTF or DIPRINTF...

I would tend to agree.

> > > (And it requires me to change 10x as many LoC.)
> > 
> > That's irritating but not a show stopper IMHO.

I agree.

> > Coming from the other angle can you omit all uses of _IPRINTF by passing
> > the context around a few more places? I'd have expected that everything
> > in xc_domain_save.c was ultimately called from xc_domain_save and
> > therefore the is a dom which could be printed?
> 
> There's a non-static function xc_map_m2p() which is defined in
> xc_domain_save.c, but called from xc_offline_page.c and
> tools/tests/mce-test/tools/xce-mceinj.c.  That should probably then be
> moved to another file in any case.
> 
> If I move that function to a different file, so that there are no
> _IPRINTF's, would that suffice?

Like Ian, I would still prefer to avoid the #undeffery.

Ian.