[PATCH v2 1/4] util/qemu-timer: Make timer_free() imply timer_del()

Peter Maydell posted 4 patches 4 years, 11 months ago
Maintainers: Alberto Garcia <berto@igalia.com>, Max Reitz <mreitz@redhat.com>, David Gibson <david@gibson.dropbear.id.au>, Dmitry Fleytman <dmitry.fleytman@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Keith Busch <kbusch@kernel.org>, Matthew Rosato <mjrosato@linux.ibm.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Klaus Jensen <its@irrelevant.dk>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Thomas Huth <thuth@redhat.com>, Amit Shah <amit@kernel.org>, John Snow <jsnow@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Jason Wang <jasowang@redhat.com>, Zhang Chen <chen.zhang@intel.com>, Alex Williamson <alex.williamson@redhat.com>, Eric Blake <eblake@redhat.com>, Samuel Thibault <samuel.thibault@ens-lyon.org>, "Michael S. Tsirkin" <mst@redhat.com>, David Hildenbrand <david@redhat.com>, Laurent Vivier <lvivier@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Juan Quintela <quintela@redhat.com>, Li Zhijian <lizhijian@cn.fujitsu.com>, Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>, Hailiang Zhang <zhang.zhanghailiang@huawei.com>, Corey Minyard <minyard@acm.org>, Greg Kurz <groug@kaod.org>, Peter Lieven <pl@kamp.de>, Gerd Hoffmann <kraxel@redhat.com>
[PATCH v2 1/4] util/qemu-timer: Make timer_free() imply timer_del()
Posted by Peter Maydell 4 years, 11 months ago
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.

Make timer_free() imply a timer_del().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/timer.h | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index bdecc5b41fe..ed84ad8f3aa 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -609,17 +609,6 @@ static inline QEMUTimer *timer_new_ms(QEMUClockType type, QEMUTimerCB *cb,
  */
 void timer_deinit(QEMUTimer *ts);
 
-/**
- * timer_free:
- * @ts: the timer
- *
- * Free a timer (it must not be on the active list)
- */
-static inline void timer_free(QEMUTimer *ts)
-{
-    g_free(ts);
-}
-
 /**
  * timer_del:
  * @ts: the timer
@@ -631,6 +620,20 @@ static inline void timer_free(QEMUTimer *ts)
  */
 void timer_del(QEMUTimer *ts);
 
+/**
+ * timer_free:
+ * @ts: the timer
+ *
+ * Free a timer. This will call timer_del() for you to remove
+ * the timer from the active list if it was still active.
+ */
+static inline void timer_free(QEMUTimer *ts)
+{
+
+    timer_del(ts);
+    g_free(ts);
+}
+
 /**
  * timer_mod_ns:
  * @ts: the timer
-- 
2.20.1


Re: [PATCH v2 1/4] util/qemu-timer: Make timer_free() imply timer_del()
Posted by Richard Henderson 4 years, 11 months ago
On 12/15/20 9:41 AM, Peter Maydell wrote:
> +static inline void timer_free(QEMUTimer *ts)
> +{
> +
> +    timer_del(ts);

Extra whitespace.  Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~