Re: [PATCH v3 1/3] qemu-keymap: Free xkb allocations

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, May 22, 2024 at 12:35:23PM +0100, Peter Maydell wrote:
> On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > This fixes LeakSanitizer complaints with xkbcommon 1.6.0.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >  qemu-keymap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/qemu-keymap.c b/qemu-keymap.c
> > index 8c80f7a4ed65..7a9f38cf9863 100644
> > --- a/qemu-keymap.c
> > +++ b/qemu-keymap.c
> > @@ -237,6 +237,9 @@ int main(int argc, char *argv[])
> >      xkb_state_unref(state);
> >      state = NULL;
> >
> > +    xkb_keymap_unref(map);
> > +    xkb_context_unref(ctx);
> > +
> >      /* add quirks */
> >      fprintf(outfile,
> >              "\n"
> 
> This is surely a sanitizer bug. We're unconditionally about
> to exit() the program here, where everything is freed, so nothing
> is leaked.

I'm not sure I'd call it a sanitizer bug, rather its expected behaviour
of sanitizers. Even if you're about to exit, its important to see info
about all memory that is not freed by that time, since it can reveal
leaks that were ongoing in the process that are valid things to fix.
To make the sanitizers usable you need to get rid of the noise. IOW,
either have to provide a file to supress reports of memory that is
expected to remain allocated, or have to free it despite being about
to exit.  Free'ing is the more maintainable strategy, as IME, supression
files get outdated over time.

So as long as the free'ing action is not unreasonably expensive, we
should just do its, so from my POV I'd

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|