[PATCH 0/2] qemu/timer: Clarify QEMUTimer new/free API

[PATCH 0/2] qemu/timer: Clarify QEMUTimer new/free API

[Philippe Mathieu-Daudé]

Update few QEMUTimer docstring and add a
sanity check during timer initialization.

Noticed trying to understand leaks in QDev
Realize -> Unrealize -> Realize transition.

Philippe Mathieu-Daudé (2):
  qemu/timer: Clarify timer_new*() must be freed with timer_free()
  qemu/timer: Sanity check timer_list in timer_init_full()

 include/qemu/timer.h | 12 +++++++++++-
 util/qemu-timer.c    |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.47.1

[PATCH 1/2] qemu/timer: Clarify timer_new*() must be freed with timer_free()

[Philippe Mathieu-Daudé]

There was not mention QEMUTimer created with timer_new*() must
be released with timer_free() instead of g_free(), because then
active timers are removed from the active list. Update the
documentation mentioning timer_free().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/timer.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index cc167bd825b..abd2204f3be 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -507,6 +507,8 @@ static inline void timer_init_ms(QEMUTimer *ts, QEMUClockType type,
  * with an AioContext---each of them runs its timer callbacks in its own
  * AioContext thread.
  *
+ * The timer returned must be freed using timer_free().
+ *
  * Returns: a pointer to the timer
  */
 static inline QEMUTimer *timer_new_full(QEMUTimerListGroup *timer_list_group,
@@ -530,6 +532,8 @@ static inline QEMUTimer *timer_new_full(QEMUTimerListGroup *timer_list_group,
  * and associate it with the default timer list for the clock type @type.
  * See timer_new_full for details.
  *
+ * The timer returned must be freed using timer_free().
+ *
  * Returns: a pointer to the timer
  */
 static inline QEMUTimer *timer_new(QEMUClockType type, int scale,
@@ -548,6 +552,8 @@ static inline QEMUTimer *timer_new(QEMUClockType type, int scale,
  * associated with the clock.
  * See timer_new_full for details.
  *
+ * The timer returned must be freed using timer_free().
+ *
  * Returns: a pointer to the newly created timer
  */
 static inline QEMUTimer *timer_new_ns(QEMUClockType type, QEMUTimerCB *cb,
@@ -566,6 +572,8 @@ static inline QEMUTimer *timer_new_ns(QEMUClockType type, QEMUTimerCB *cb,
  * associated with the clock.
  * See timer_new_full for details.
  *
+ * The timer returned must be freed using timer_free().
+ *
  * Returns: a pointer to the newly created timer
  */
 static inline QEMUTimer *timer_new_us(QEMUClockType type, QEMUTimerCB *cb,
@@ -584,6 +592,8 @@ static inline QEMUTimer *timer_new_us(QEMUClockType type, QEMUTimerCB *cb,
  * associated with the clock.
  * See timer_new_full for details.
  *
+ * The timer returned must be freed using timer_free().
+ *
  * Returns: a pointer to the newly created timer
  */
 static inline QEMUTimer *timer_new_ms(QEMUClockType type, QEMUTimerCB *cb,
-- 
2.47.1

Re: [PATCH 1/2] qemu/timer: Clarify timer_new*() must be freed with timer_free()

[Daniel P. Berrangé]

On Sat, Jan 25, 2025 at 07:24:24PM +0100, Philippe Mathieu-Daudé wrote:
> There was not mention QEMUTimer created with timer_new*() must
> be released with timer_free() instead of g_free(), because then
> active timers are removed from the active list. Update the
> documentation mentioning timer_free().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qemu/timer.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH 2/2] qemu/timer: Sanity check timer_list in timer_init_full()

[Philippe Mathieu-Daudé]

Ensure we are not re-initializing a QEMUTimer already added
to an active list. timer_init*() functions expect either
a recently created and zeroed QEMUTimer, or one previously
free'd with timer_free().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/timer.h | 2 +-
 util/qemu-timer.c    | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index abd2204f3be..4717693f950 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -407,7 +407,7 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup *tlg);
  * (or default timer list group, if NULL).
  * The caller is responsible for allocating the memory.
  *
- * You need not call an explicit deinit call. Simply make
+ * You need not call an explicit timer_deinit() call. Simply make
  * sure it is not on a list with timer_del.
  */
 void timer_init_full(QEMUTimer *ts,
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 0e8a453eaa1..058cae6e487 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -354,6 +354,7 @@ void timer_init_full(QEMUTimer *ts,
     if (!timer_list_group) {
         timer_list_group = &main_loop_tlg;
     }
+    assert(ts->timer_list == NULL);
     ts->timer_list = timer_list_group->tl[type];
     ts->cb = cb;
     ts->opaque = opaque;
-- 
2.47.1

Re: [PATCH 2/2] qemu/timer: Sanity check timer_list in timer_init_full()

[Daniel P. Berrangé]

On Sat, Jan 25, 2025 at 07:24:25PM +0100, Philippe Mathieu-Daudé wrote:
> Ensure we are not re-initializing a QEMUTimer already added
> to an active list. timer_init*() functions expect either
> a recently created and zeroed QEMUTimer, or one previously
> free'd with timer_free().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qemu/timer.h | 2 +-
>  util/qemu-timer.c    | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] qemu/timer: Sanity check timer_list in timer_init_full()

[Michael Tokarev]

25.01.2025 21:24, Philippe Mathieu-Daudé wrote:

> - * You need not call an explicit deinit call. Simply make
> + * You need not call an explicit timer_deinit() call. Simply make
>    * sure it is not on a list with timer_del.

Reworded this as "You need not call timer_deinit() explicitly. Simply make..."

and applied to trivial-patches tree.

Thanks,

/mjt

Re: [PATCH 2/2] qemu/timer: Sanity check timer_list in timer_init_full()

[Michael Tokarev]

09.02.2025 12:37, Michael Tokarev wrote:
> 25.01.2025 21:24, Philippe Mathieu-Daudé wrote:
> 
>> - * You need not call an explicit deinit call. Simply make
>> + * You need not call an explicit timer_deinit() call. Simply make
>>    * sure it is not on a list with timer_del.
> 
> Reworded this as "You need not call timer_deinit() explicitly. Simply make..."

"You don't need to call timer_deinit() explicitly.", actually.

This breaks quite a lot of CI tests: https://gitlab.com/mjt0k/qemu/-/pipelines/1662551717

Re: [PATCH 2/2] qemu/timer: Sanity check timer_list in timer_init_full()

[Philippe Mathieu-Daudé]

On 9/2/25 10:41, Michael Tokarev wrote:
> 09.02.2025 12:37, Michael Tokarev wrote:
>> 25.01.2025 21:24, Philippe Mathieu-Daudé wrote:
>>
>>> - * You need not call an explicit deinit call. Simply make
>>> + * You need not call an explicit timer_deinit() call. Simply make
>>>    * sure it is not on a list with timer_del.
>>
>> Reworded this as "You need not call timer_deinit() explicitly. Simply 
>> make..."
> 
> "You don't need to call timer_deinit() explicitly.", actually.
> 
> This breaks quite a lot of CI tests: https://gitlab.com/mjt0k/qemu/-/ 
> pipelines/1662551717

Do sorry, I only tested on a specific config and missed all the
other errors :/

Re: [PATCH 0/2] qemu/timer: Clarify QEMUTimer new/free API

[Philippe Mathieu-Daudé]

ping?

On 25/1/25 19:24, Philippe Mathieu-Daudé wrote:
> Update few QEMUTimer docstring and add a
> sanity check during timer initialization.
> 
> Noticed trying to understand leaks in QDev
> Realize -> Unrealize -> Realize transition.
> 
> Philippe Mathieu-Daudé (2):
>    qemu/timer: Clarify timer_new*() must be freed with timer_free()
>    qemu/timer: Sanity check timer_list in timer_init_full()
> 
>   include/qemu/timer.h | 12 +++++++++++-
>   util/qemu-timer.c    |  1 +
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/2] qemu/timer: Clarify QEMUTimer new/free API

[Philippe Mathieu-Daudé]

Cc'ing more developers.

On 4/2/25 22:44, Philippe Mathieu-Daudé wrote:
> ping?
> 
> On 25/1/25 19:24, Philippe Mathieu-Daudé wrote:
>> Update few QEMUTimer docstring and add a
>> sanity check during timer initialization.
>>
>> Noticed trying to understand leaks in QDev
>> Realize -> Unrealize -> Realize transition.
>>
>> Philippe Mathieu-Daudé (2):
>>    qemu/timer: Clarify timer_new*() must be freed with timer_free()
>>    qemu/timer: Sanity check timer_list in timer_init_full()
>>
>>   include/qemu/timer.h | 12 +++++++++++-
>>   util/qemu-timer.c    |  1 +
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>