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 <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
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.
+ */
+static inline QEMUClockType monitor_get_clock(void)
+{
+ /*
+ * 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);
--
2.17.1
Peter Xu <peterx@redhat.com> 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 <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 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 <armbru@redhat.com>
On Fri, Jun 08, 2018 at 07:38:11AM +0200, Markus Armbruster wrote: [...] > > +/* > > + * 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(). Oh, another Chinglish from me. :( > > > + */ > > +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. That'll be appreciated. Both changes work for me. [...] > With at least a suitable fix for the comment: > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks! -- Peter Xu
Peter Xu <peterx@redhat.com> writes: > On Fri, Jun 08, 2018 at 07:38:11AM +0200, Markus Armbruster wrote: > > [...] > >> > +/* >> > + * 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(). > > Oh, another Chinglish from me. :( Useful code, occasional Chinglish, the humility to go with it, and a sense of humor --- you're doing fine. [...]
On Fri, Jun 08, 2018 at 09:09:57AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Fri, Jun 08, 2018 at 07:38:11AM +0200, Markus Armbruster wrote: > > > > [...] > > > >> > +/* > >> > + * 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(). > > > > Oh, another Chinglish from me. :( > > Useful code, occasional Chinglish, the humility to go with it, and a > sense of humor --- you're doing fine. Very high evaluation. Thank you. :) -- Peter Xu
On Fri, Jun 08, 2018 at 11:55:09AM +0800, Peter Xu wrote: > 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 <armbru@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) With Markus' suggestion: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
© 2016 - 2026 Red Hat, Inc.