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 <pbonz...@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
