Re: [PATCH 0/3] qemu-timer: Make timer_free() imply timer_del()

[Peter Maydell]

On Tue, 15 Dec 2020 at 10:07, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 14/12/20 21:30, Peter Maydell wrote:
> > Currently timer_free() is a simple wrapper for g_free().  This means
> > that the timer being freed must not be currently active, as otherwise
> > QEMU might crash later when the active list is processed and still
> > has a pointer to freed memory on it.  As a result almost all calls to
> > timer_free() are preceded by a timer_del() call, as can be seen in
> > the output of
> >    git grep -B1 '\<timer_free\>'
> >
> > This is unfortunate API design as it makes it easy to accidentally
> > misuse (by forgetting the timer_del()), and the correct use is
> > annoyingly verbose.
> >
> > Patch 1 makes timer_free() call timer_del() if the timer is active.
> > Patch 2 is a Coccinelle script to remove any timer_del() calls
> > that are immediately before the timer_free().
> > Patch 3 is the result of running the Coccinelle script on the
> > whole tree.
> >
> > I could split up patch 3, but for 58 deleted lines over 42 files
> > created entirely automatedly, it seemed to me to be simpler as one
> > patch.
>
> Looks good.  Even better would be to make timers embedded in structs
> rather than heap-allocated

We do support that -- you use timer_init*() instead of timer_new*(),
and maybe timer_deinit(). It's just that the existing style is very
heavily towards heap-allocation because there's a lot of older
code that was written that way. I'm not sure whether changing
a heap-allocated timer to an embedded timer is a migration
break; if it isn't we could in theory convert some existing code.

>, but this patch makes things a little bit
> better in that respect since we have a 1:1 correspondence
> (timer_{new->init} and timer_{free->del}) between the APIs.
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

thanks
-- PMM