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

[PATCH] 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 complains from valgrind.

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

The second requires quite a bit of code motion to fix sensibly.
 * The three logging functions are moved up.  The destroy() function has been
   modified to be less antisocial.
 * The global 'logger' is initialised in place. This requires changing the
   indirection of its use in 5 locations.
 * The struct xentoollog_logger_tellparent and function
   createlogger_tellparent() are unused and removed.

This completely removes any memory allocation associated with logging

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

---

The macro XTL_NEW_LOGGER() is a special kind of crazy, requiring types and
functions with magic names to appear in scope.  It is also complete overkill
for libxl-save-helpers use.
---
 tools/libxl/libxl_save_helper.c |   87 +++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 50 deletions(-)

diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
index 880565e..7daff8e 100644
--- a/tools/libxl/libxl_save_helper.c
+++ b/tools/libxl/libxl_save_helper.c
@@ -47,10 +47,41 @@
 #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)
+{
+}
+
 /*----- 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 -----*/
@@ -61,7 +92,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);
 }
 
@@ -86,45 +117,6 @@ 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;
-    return XTL_NEW_LOGGER(tellparent, newlogger);
-}
-
 /*----- helper functions called by autogenerated stubs -----*/
 
 unsigned char * helper_allocbuf(int len, void *user)
@@ -184,22 +176,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] tools/libxl: Improvements to libxl-save-helper when using valgrind

[Ian Jackson]

Andrew Cooper writes ("[PATCH] 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 complains from valgrind.
> 
> The first is easy; close the interface to libxc when done with it.
> 
> The second requires quite a bit of code motion to fix sensibly.
>  * The three logging functions are moved up.

Can you split the pure code motion into a separate patch ?  That
always makes things much easier to review.

>  * The destroy() function has been modified to be less antisocial.

Why ?  Who calls the destroy function ?  It's even less appropriate to
destroy this thing now that it's allocated statically.

>  * The global 'logger' is initialised in place. This requires changing the
>    indirection of its use in 5 locations.

If you wrote:

  +static xentoollog_logger logger[1] = {{

then the call sites could remain unchanged.

> This completely removes any memory allocation associated with logging

You mean, with the logger instance.  Actual log messages involve an
allocation for every message.

Ian.

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

[Andrew Cooper]

On 10/04/14 19:09, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] 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 complains from valgrind.
>>
>> The first is easy; close the interface to libxc when done with it.
>>
>> The second requires quite a bit of code motion to fix sensibly.
>>  * The three logging functions are moved up.
> Can you split the pure code motion into a separate patch ?  That
> always makes things much easier to review.

Ok

>
>>  * The destroy() function has been modified to be less antisocial.
> Why ?  Who calls the destroy function ?  It's even less appropriate to
> destroy this thing now that it's allocated statically.

Only on manual calls to xtl_logger_destroy(), which don't check for
NULLness of the function pointer before calling it.

I considered dropping it and wasn't fussed either way - I shall drop for v2.

>
>>  * The global 'logger' is initialised in place. This requires changing the
>>    indirection of its use in 5 locations.
> If you wrote:
>
>   +static xentoollog_logger logger[1] = {{
>
> then the call sites could remain unchanged.

I did that originally, decided it was ugly.  Given that changing the
indirection was small compared to the rest, I opted to fix it nicely.

>
>> This completely removes any memory allocation associated with logging
> You mean, with the logger instance.  Actual log messages involve an
> allocation for every message.
>
> Ian.

I will update the message.

~Andrew

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

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] tools/libxl: Improvements to libxl-save-helper when using valgrind"):
> On 10/04/14 19:09, Ian Jackson wrote:
> > Andrew Cooper writes ("[PATCH] tools/libxl: Improvements to libxl-save-helper when using valgrind"):
> >>  * The destroy() function has been modified to be less antisocial.
> >
> > Why ?  Who calls the destroy function ?  It's even less appropriate to
> > destroy this thing now that it's allocated statically.
> 
> Only on manual calls to xtl_logger_destroy(), which don't check for
> NULLness of the function pointer before calling it.

This logger is never destroyed, though, is it.  When you say "manual
calls" you don't mean the end user.  And there are no such calls on
this logger.

The reason I provided a function which calls abort() is that in case
someone did foolishly try to destroy it, it produces a better stack
trace.

Thanks,
Ian.

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

[Andrew Cooper]

On 11/04/14 12:06, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH] tools/libxl: Improvements to libxl-save-helper when using valgrind"):
>> On 10/04/14 19:09, Ian Jackson wrote:
>>> Andrew Cooper writes ("[PATCH] tools/libxl: Improvements to libxl-save-helper when using valgrind"):
>>>>  * The destroy() function has been modified to be less antisocial.
>>> Why ?  Who calls the destroy function ?  It's even less appropriate to
>>> destroy this thing now that it's allocated statically.
>> Only on manual calls to xtl_logger_destroy(), which don't check for
>> NULLness of the function pointer before calling it.
> This logger is never destroyed, though, is it.

Correct

>   When you say "manual
> calls" you don't mean the end user.

I mean the owner of the logger explicitly calling "xtl_logger_destroy()"
on it.

>   And there are no such calls on
> this logger.

Correct.

>
> The reason I provided a function which calls abort() is that in case
> someone did foolishly try to destroy it, it produces a better stack
> trace.

I suppose that is reasonable.  There is nothing which prevents some
library code from attempting to destroy the logger in xch.

The original 'antisocial' comment was when I tried to actually destroy
the logger in the hope that it would free() the memory it allocated,
before finding that it didn't work and having to look deeper for the
leaked allocation.

I shall respin a v4 leaving this in.

~Andrew