Re: [Qemu-devel] [PATCH v10 5/7] monitor: remove event_clock_type

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> Instead, use a dynamic function to detect which clock we'll use.  The
> problem is that the old code will let monitor initialization depend on
> configure_accelerator() (that's where qtest_enabled() start to take
> effect).  After this change, we don't have such a dependency any more.
> We just need to make sure configure_accelerator() is called when we
> start to use it.  Now it's only used in monitor_qapi_event_queue() and
> monitor_qapi_event_handler(), so we're good.
>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  monitor.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index fa9c35631f..2bda0dfa12 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -281,8 +281,6 @@ QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>  
>  Monitor *cur_mon;
>  
> -static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
> -
>  static void monitor_command_cb(void *opaque, const char *cmdline,
>                                 void *readline_opaque);
>  
> @@ -309,6 +307,19 @@ static inline bool monitor_is_hmp_non_interactive(const 
> Monitor *mon)
>      return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
>  }
>  
> +/*
> + * This should never be called before configure_accelerator() since
> + * only until then could we know whether qtest was enabled or not.

Uh, we know it after then, not until then.  What about

   /*
    * Return the clock to use for recording an event's time.
    * Beware: result is invalid before configure_accelerator().

> + */
> +static inline QEMUClockType monitor_get_clock(void)

Suggest to rename to monitor_get_event_clock(), or just
get_event_clock().

Happy to apply touch-ups on commit.

> +{
> +    /*
> +     * This allows us to perform tests on the monitor queues to verify
> +     * that the rate limits are enforced.
> +     */
> +    return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
> +}
> +
>  /**
>   * Is the current monitor, if any, a QMP monitor?
>   */
> @@ -632,7 +643,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, 
> Error **errp)
>               * monitor_qapi_event_handler() in evconf->rate ns.  Any
>               * events arriving before then will be delayed until then.
>               */
> -            int64_t now = qemu_clock_get_ns(event_clock_type);
> +            int64_t now = qemu_clock_get_ns(monitor_get_clock());
>  
>              monitor_qapi_event_emit(event, qdict);
>  
> @@ -640,7 +651,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, 
> Error **errp)
>              evstate->event = event;
>              evstate->data = qobject_ref(data);
>              evstate->qdict = NULL;
> -            evstate->timer = timer_new_ns(event_clock_type,
> +            evstate->timer = timer_new_ns(monitor_get_clock(),
>                                            monitor_qapi_event_handler,
>                                            evstate);
>              g_hash_table_add(monitor_qapi_event_state, evstate);
> @@ -665,7 +676,7 @@ static void monitor_qapi_event_handler(void *opaque)
>      qemu_mutex_lock(&monitor_lock);
>  
>      if (evstate->qdict) {
> -        int64_t now = qemu_clock_get_ns(event_clock_type);
> +        int64_t now = qemu_clock_get_ns(monitor_get_clock());
>  
>          monitor_qapi_event_emit(evstate->event, evstate->qdict);
>          qobject_unref(evstate->qdict);
> @@ -721,10 +732,6 @@ static gboolean qapi_event_throttle_equal(const void *a, 
> const void *b)
>  
>  static void monitor_qapi_event_init(void)
>  {
> -    if (qtest_enabled()) {
> -        event_clock_type = QEMU_CLOCK_VIRTUAL;
> -    }
> -
>      monitor_qapi_event_state = g_hash_table_new(qapi_event_throttle_hash,
>                                                  qapi_event_throttle_equal);
>      qmp_event_set_func_emit(monitor_qapi_event_queue);

With at least a suitable fix for the comment:
Reviewed-by: Markus Armbruster <address@hidden>