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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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