[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 0/4] qemu-timer: Make timer_free() imply timer_del()
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 0/4] qemu-timer: Make timer_free() imply timer_del() |
Date: |
Tue, 5 Jan 2021 11:54:05 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
On 12/15/20 4:41 PM, 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.
> Patch 4 makes a by-hand cleanup of a bit of the Arm CPU code
> not caught by Coccinelle because it included a pointless
> timer_deinit() between timer_del() and timer_free().
>
> 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.
>
> Changes v1->v2:
> * patch 1: unconditionally call timer_del() rather than trying
> to be clever
> * patch 4: new patch
>
> thanks
> -- PMM
>
> Peter Maydell (4):
> util/qemu-timer: Make timer_free() imply timer_del()
> scripts/coccinelle: New script to remove unnecessary timer_del() calls
> Remove superfluous timer_del() calls
> target/arm: Remove timer_del()/timer_deinit() before timer_free()
>
> scripts/coccinelle/timer-del-timer-free.cocci | 18 +++++++++++++
> include/qemu/timer.h | 25 +++++++++++--------
> 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/arm/cpu.c | 2 --
> target/s390x/cpu.c | 2 --
> ui/console.c | 1 -
> ui/spice-core.c | 1 -
> util/throttle.c | 1 -
> 45 files changed, 32 insertions(+), 71 deletions(-)
> create mode 100644 scripts/coccinelle/timer-del-timer-free.cocci
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
- Re: [PATCH v2 0/4] qemu-timer: Make timer_free() imply timer_del(),
Philippe Mathieu-Daudé <=