From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] tools/libxl: Improvements to libxl-save-helper when using valgrind Date: Thu, 10 Apr 2014 19:14:11 +0100 Message-ID: <5346DF73.3050705@citrix.com> References: <1397152331-16769-1-git-send-email-andrew.cooper3@citrix.com> <21318.56947.346171.333514@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21318.56947.346171.333514@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org 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