[PATCH v4 1/2] libxl/save-helper: Code motion of logging functions

[PATCH v4 1/2] libxl/save-helper: Code motion of logging functions

[Andrew Cooper]

... in preparation for a subsequent functional fix

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

---
v4: Move tellparent_destroy() as well
---
 tools/libxl/libxl_save_helper.c |   58 +++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
index 880565e..d6fa5dd 100644
--- a/tools/libxl/libxl_save_helper.c
+++ b/tools/libxl/libxl_save_helper.c
@@ -47,6 +47,35 @@
 #include "xenguest.h"
 #include "_libxl_save_msgs_helper.h"
 
+/*----- logger -----*/
+
+static void tellparent_vmessage(xentoollog_logger *logger_in,
+                                xentoollog_level level,
+                                int errnoval,
+                                const char *context,
+                                const char *format,
+                                va_list al)
+{
+    char *formatted;
+    int r = vasprintf(&formatted, format, al);
+    if (r < 0) { perror("memory allocation failed during logging"); exit(-1); }
+    helper_stub_log(level, errnoval, context, formatted, 0);
+    free(formatted);
+}
+
+static void tellparent_progress(struct xentoollog_logger *logger_in,
+                                const char *context,
+                                const char *doing_what, int percent,
+                                unsigned long done, unsigned long total)
+{
+    helper_stub_progress(context, doing_what, done, total, 0);
+}
+
+static void tellparent_destroy(struct xentoollog_logger *logger_in)
+{
+    abort();
+}
+
 /*----- globals -----*/
 
 static const char *program = "libxl-save-helper";
@@ -86,39 +115,10 @@ static void *xmalloc(size_t sz)
     return r;
 }
 
-/*----- logger -----*/
-
 typedef struct {
     xentoollog_logger vtable;
 } xentoollog_logger_tellparent;
 
-static void tellparent_vmessage(xentoollog_logger *logger_in,
-                                xentoollog_level level,
-                                int errnoval,
-                                const char *context,
-                                const char *format,
-                                va_list al)
-{
-    char *formatted;
-    int r = vasprintf(&formatted, format, al);
-    if (r < 0) { perror("memory allocation failed during logging"); exit(-1); }
-    helper_stub_log(level, errnoval, context, formatted, 0);
-    free(formatted);
-}
-
-static void tellparent_progress(struct xentoollog_logger *logger_in,
-                                const char *context,
-                                const char *doing_what, int percent,
-                                unsigned long done, unsigned long total)
-{
-    helper_stub_progress(context, doing_what, done, total, 0);
-}
-
-static void tellparent_destroy(struct xentoollog_logger *logger_in)
-{
-    abort();
-}
-
 static xentoollog_logger_tellparent *createlogger_tellparent(void)
 {
     xentoollog_logger_tellparent newlogger;
-- 
1.7.10.4

[PATCH v4 2/2] tools/libxl: Improvements to libxl-save-helper when using valgrind

[Andrew Cooper]

Fix two unfree()'d allocations in libxl-save-helper, to get them out of the
way of other legitimate complaints from valgrind.

The first is easy; close the interface to libxc when done with it.

The second can be fixed by removing the complexity of creating the logging
instance.  Initialise the global 'logger' in place rather than as an
allocation, which requires changing the indirection of its use in 5 locations.

struct xentoollog_logger_tellparent and function createlogger_tellparent() are
now unused and removed.

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

---
v4: Leave tellparent_destroy() around
v3: Better spelling in commit message
---
 tools/libxl/libxl_save_helper.c |   31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
index d6fa5dd..c36314c 100644
--- a/tools/libxl/libxl_save_helper.c
+++ b/tools/libxl/libxl_save_helper.c
@@ -79,7 +79,11 @@ static void tellparent_destroy(struct xentoollog_logger *logger_in)
 /*----- globals -----*/
 
 static const char *program = "libxl-save-helper";
-static xentoollog_logger *logger;
+static xentoollog_logger logger = {
+    tellparent_vmessage,
+    tellparent_progress,
+    tellparent_destroy,
+};
 static xc_interface *xch;
 
 /*----- error handling -----*/
@@ -90,7 +94,7 @@ static void fail(int errnoval, const char *fmt, ...)
 {
     va_list al;
     va_start(al,fmt);
-    xtl_logv(logger,XTL_ERROR,errnoval,program,fmt,al);
+    xtl_logv(&logger,XTL_ERROR,errnoval,program,fmt,al);
     exit(-1);
 }
 
@@ -115,16 +119,6 @@ static void *xmalloc(size_t sz)
     return r;
 }
 
-typedef struct {
-    xentoollog_logger vtable;
-} xentoollog_logger_tellparent;
-
-static xentoollog_logger_tellparent *createlogger_tellparent(void)
-{
-    xentoollog_logger_tellparent newlogger;
-    return XTL_NEW_LOGGER(tellparent, newlogger);
-}
-
 /*----- helper functions called by autogenerated stubs -----*/
 
 unsigned char * helper_allocbuf(int len, void *user)
@@ -184,22 +178,17 @@ static int toolstack_save_cb(uint32_t domid, uint8_t **buf,
 }
 
 static void startup(const char *op) {
-    logger = (xentoollog_logger*)createlogger_tellparent();
-    if (!logger) {
-        fprintf(stderr, "%s: cannot initialise logger\n", program);
-        exit(-1);
-    }
-
-    xtl_log(logger,XTL_DEBUG,0,program,"starting %s",op);
+    xtl_log(&logger,XTL_DEBUG,0,program,"starting %s",op);
 
-    xch = xc_interface_open(logger,logger,0);
+    xch = xc_interface_open(&logger,&logger,0);
     if (!xch) fail(errno,"xc_interface_open failed");
 }
 
 static void complete(int retval) {
     int errnoval = retval ? errno : 0; /* suppress irrelevant errnos */
-    xtl_log(logger,XTL_DEBUG,errnoval,program,"complete r=%d",retval);
+    xtl_log(&logger,XTL_DEBUG,errnoval,program,"complete r=%d",retval);
     helper_stub_complete(retval,errnoval,0);
+    xc_interface_close(xch);
     exit(0);
 }
 
-- 
1.7.10.4

Re: [PATCH v4 1/2] libxl/save-helper: Code motion of logging functions

[Ian Jackson]

Andrew Cooper writes ("[PATCH v4 1/2] libxl/save-helper: Code motion of logging functions"):
> ... in preparation for a subsequent functional fix

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

Re: [PATCH v4 2/2] tools/libxl: Improvements to libxl-save-helper when using valgrind

[Ian Jackson]

Andrew Cooper writes ("[PATCH v4 2/2] tools/libxl: Improvements to libxl-save-helper when using valgrind"):
> Fix two unfree()'d allocations in libxl-save-helper, to get them out of the
> way of other legitimate complaints from valgrind.
> 
> The first is easy; close the interface to libxc when done with it.
> 
> The second can be fixed by removing the complexity of creating the logging
> instance.  Initialise the global 'logger' in place rather than as an
> allocation, which requires changing the indirection of its use in 5 locations.
> 
> struct xentoollog_logger_tellparent and function createlogger_tellparent() are
> now unused and removed.

Great, thanks.

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

Re: [PATCH v4 2/2] tools/libxl: Improvements to libxl-save-helper when using valgrind

[Ian Campbell]

On Fri, 2014-04-11 at 16:57 +0100, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH v4 2/2] tools/libxl: Improvements to libxl-save-helper when using valgrind"):
> > Fix two unfree()'d allocations in libxl-save-helper, to get them out of the
> > way of other legitimate complaints from valgrind.
> > 
> > The first is easy; close the interface to libxc when done with it.
> > 
> > The second can be fixed by removing the complexity of creating the logging
> > instance.  Initialise the global 'logger' in place rather than as an
> > allocation, which requires changing the indirection of its use in 5 locations.
> > 
> > struct xentoollog_logger_tellparent and function createlogger_tellparent() are
> > now unused and removed.
> 
> Great, thanks.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

APplied both.