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.

thanks
-- PMM

Peter Maydell (3):
  util/qemu-timer: Make timer_free() imply timer_del()
  scripts/coccinelle: New script to remove unnecessary timer_del() calls
  Remove superfluous timer_del() calls

 scripts/coccinelle/timer-del-timer-free.cocci | 18 +++++++++++++
 include/qemu/timer.h                          | 27 +++++++++++--------
 block/iscsi.c                                 |  2 --
 block/nbd.c                                   |  1 -
 block/qcow2.c                                 |  1 -
 hw/block/nvme.c                               |  2 --
 hw/char/serial.c                              |  2 --
 hw/char/virtio-serial-bus.c                   |  2 --
 hw/ide/core.c                                 |  1 -
 hw/input/hid.c                                |  1 -
 hw/intc/apic.c                                |  1 -
 hw/intc/ioapic.c                              |  1 -
 hw/ipmi/ipmi_bmc_extern.c                     |  1 -
 hw/net/e1000.c                                |  3 ---
 hw/net/e1000e_core.c                          |  8 ------
 hw/net/pcnet-pci.c                            |  1 -
 hw/net/rtl8139.c                              |  1 -
 hw/net/spapr_llan.c                           |  1 -
 hw/net/virtio-net.c                           |  2 --
 hw/s390x/s390-pci-inst.c                      |  1 -
 hw/sd/sd.c                                    |  1 -
 hw/sd/sdhci.c                                 |  2 --
 hw/usb/dev-hub.c                              |  1 -
 hw/usb/hcd-ehci.c                             |  1 -
 hw/usb/hcd-ohci-pci.c                         |  1 -
 hw/usb/hcd-uhci.c                             |  1 -
 hw/usb/hcd-xhci.c                             |  1 -
 hw/usb/redirect.c                             |  1 -
 hw/vfio/display.c                             |  1 -
 hw/virtio/vhost-vsock-common.c                |  1 -
 hw/virtio/virtio-balloon.c                    |  1 -
 hw/virtio/virtio-rng.c                        |  1 -
 hw/watchdog/wdt_diag288.c                     |  1 -
 hw/watchdog/wdt_i6300esb.c                    |  1 -
 migration/colo.c                              |  1 -
 monitor/hmp-cmds.c                            |  1 -
 net/announce.c                                |  1 -
 net/colo-compare.c                            |  1 -
 net/slirp.c                                   |  1 -
 replay/replay-debugging.c                     |  1 -
 target/s390x/cpu.c                            |  2 --
 ui/console.c                                  |  1 -
 ui/spice-core.c                               |  1 -
 util/throttle.c                               |  1 -
 44 files changed, 34 insertions(+), 69 deletions(-)
 create mode 100644 scripts/coccinelle/timer-del-timer-free.cocci

-- 
2.20.1

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, 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>

Paolo

> 
> Peter Maydell (3):
>    util/qemu-timer: Make timer_free() imply timer_del()
>    scripts/coccinelle: New script to remove unnecessary timer_del() calls
>    Remove superfluous timer_del() calls
> 
>   scripts/coccinelle/timer-del-timer-free.cocci | 18 +++++++++++++
>   include/qemu/timer.h                          | 27 +++++++++++--------
>   block/iscsi.c                                 |  2 --
>   block/nbd.c                                   |  1 -
>   block/qcow2.c                                 |  1 -
>   hw/block/nvme.c                               |  2 --
>   hw/char/serial.c                              |  2 --
>   hw/char/virtio-serial-bus.c                   |  2 --
>   hw/ide/core.c                                 |  1 -
>   hw/input/hid.c                                |  1 -
>   hw/intc/apic.c                                |  1 -
>   hw/intc/ioapic.c                              |  1 -
>   hw/ipmi/ipmi_bmc_extern.c                     |  1 -
>   hw/net/e1000.c                                |  3 ---
>   hw/net/e1000e_core.c                          |  8 ------
>   hw/net/pcnet-pci.c                            |  1 -
>   hw/net/rtl8139.c                              |  1 -
>   hw/net/spapr_llan.c                           |  1 -
>   hw/net/virtio-net.c                           |  2 --
>   hw/s390x/s390-pci-inst.c                      |  1 -
>   hw/sd/sd.c                                    |  1 -
>   hw/sd/sdhci.c                                 |  2 --
>   hw/usb/dev-hub.c                              |  1 -
>   hw/usb/hcd-ehci.c                             |  1 -
>   hw/usb/hcd-ohci-pci.c                         |  1 -
>   hw/usb/hcd-uhci.c                             |  1 -
>   hw/usb/hcd-xhci.c                             |  1 -
>   hw/usb/redirect.c                             |  1 -
>   hw/vfio/display.c                             |  1 -
>   hw/virtio/vhost-vsock-common.c                |  1 -
>   hw/virtio/virtio-balloon.c                    |  1 -
>   hw/virtio/virtio-rng.c                        |  1 -
>   hw/watchdog/wdt_diag288.c                     |  1 -
>   hw/watchdog/wdt_i6300esb.c                    |  1 -
>   migration/colo.c                              |  1 -
>   monitor/hmp-cmds.c                            |  1 -
>   net/announce.c                                |  1 -
>   net/colo-compare.c                            |  1 -
>   net/slirp.c                                   |  1 -
>   replay/replay-debugging.c                     |  1 -
>   target/s390x/cpu.c                            |  2 --
>   ui/console.c                                  |  1 -
>   ui/spice-core.c                               |  1 -
>   util/throttle.c                               |  1 -
>   44 files changed, 34 insertions(+), 69 deletions(-)
>   create mode 100644 scripts/coccinelle/timer-del-timer-free.cocci
> 

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