Instead, use a dynamic function to detect which clock we'll use. The
problem is that the old code will let monitor initialization depends on
qtest_enabled(). After this change, we don't have such a dependency any
more.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/monitor.c b/monitor.c
index 2504696d76..bd9ab5597f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -282,8 +282,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);
@@ -310,6 +308,15 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
}
+static inline QEMUClockType monitor_get_clock(void)
+{
+ if (qtest_enabled()) {
+ return QEMU_CLOCK_VIRTUAL;
+ } else {
+ return QEMU_CLOCK_REALTIME;
+ }
+}
+
/**
* Is the current monitor, if any, a QMP monitor?
*/
@@ -633,7 +640,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);
@@ -641,7 +648,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);
@@ -666,7 +673,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);
@@ -722,10 +729,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.0
On Tue, May 29, 2018 at 01:57:53PM +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 depends on > qtest_enabled(). After this change, we don't have such a dependency any > more. There is a hidden dependency: monitor_get_clock() returns the wrong value before main() has processed command-line arguments. Where is the guarantee that monitor_get_clock() is never called too early? At the least, monitor_get_clock() should call abort(3) if invoked too early. Even better would be an interface that cannot be used incorrectly.
On Wed, May 30, 2018 at 05:35:52PM +0100, Stefan Hajnoczi wrote:
> On Tue, May 29, 2018 at 01:57:53PM +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 depends on
> > qtest_enabled(). After this change, we don't have such a dependency any
> > more.
>
> There is a hidden dependency:
>
> monitor_get_clock() returns the wrong value before main() has
> processed command-line arguments.
To be more explicit:
monitor_get_clock() returns the wrong value before accelerator is
correctly setup (in configure_accelerator()).
Since the only thing that matters here is whether we're using the
qtest accelerator.
>
> Where is the guarantee that monitor_get_clock() is never called too
> early?
You are right, there is no guarantee except from programming POV.
It's only used in:
monitor_qapi_event_queue
monitor_qapi_event_handler
These two functions will never be called until accelerator is setup.
>
> At the least, monitor_get_clock() should call abort(3) if invoked too
> early. Even better would be an interface that cannot be used
> incorrectly.
Maybe then I should export the accel_initialised variable in
configure_accelerator() and then I assert with that. But that'll
further expand the series a bit.
Or, I can also mention above in the commit message to explain that a
bit.
What would you prefer?
Thanks,
--
Peter Xu
On Thu, May 31, 2018 at 12:11:47PM +0800, Peter Xu wrote: > On Wed, May 30, 2018 at 05:35:52PM +0100, Stefan Hajnoczi wrote: > > On Tue, May 29, 2018 at 01:57:53PM +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 depends on > > > qtest_enabled(). After this change, we don't have such a dependency any > > > more. > > > > There is a hidden dependency: > > > > monitor_get_clock() returns the wrong value before main() has > > processed command-line arguments. > > To be more explicit: > > monitor_get_clock() returns the wrong value before accelerator is > correctly setup (in configure_accelerator()). > > Since the only thing that matters here is whether we're using the > qtest accelerator. > > > > > Where is the guarantee that monitor_get_clock() is never called too > > early? > > You are right, there is no guarantee except from programming POV. > It's only used in: > > monitor_qapi_event_queue > monitor_qapi_event_handler > > These two functions will never be called until accelerator is setup. > > > > > At the least, monitor_get_clock() should call abort(3) if invoked too > > early. Even better would be an interface that cannot be used > > incorrectly. > > Maybe then I should export the accel_initialised variable in > configure_accelerator() and then I assert with that. But that'll > further expand the series a bit. > > Or, I can also mention above in the commit message to explain that a > bit. Documentation is okay, but please do it in the code, not the commit message. That way anyone looking at monitor_get_clock() will be aware of the constraint. Stefan
On Thu, May 31, 2018 at 09:23:24AM +0100, Stefan Hajnoczi wrote: > On Thu, May 31, 2018 at 12:11:47PM +0800, Peter Xu wrote: > > On Wed, May 30, 2018 at 05:35:52PM +0100, Stefan Hajnoczi wrote: > > > On Tue, May 29, 2018 at 01:57:53PM +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 depends on > > > > qtest_enabled(). After this change, we don't have such a dependency any > > > > more. > > > > > > There is a hidden dependency: > > > > > > monitor_get_clock() returns the wrong value before main() has > > > processed command-line arguments. > > > > To be more explicit: > > > > monitor_get_clock() returns the wrong value before accelerator is > > correctly setup (in configure_accelerator()). > > > > Since the only thing that matters here is whether we're using the > > qtest accelerator. > > > > > > > > Where is the guarantee that monitor_get_clock() is never called too > > > early? > > > > You are right, there is no guarantee except from programming POV. > > It's only used in: > > > > monitor_qapi_event_queue > > monitor_qapi_event_handler > > > > These two functions will never be called until accelerator is setup. > > > > > > > > At the least, monitor_get_clock() should call abort(3) if invoked too > > > early. Even better would be an interface that cannot be used > > > incorrectly. > > > > Maybe then I should export the accel_initialised variable in > > configure_accelerator() and then I assert with that. But that'll > > further expand the series a bit. > > > > Or, I can also mention above in the commit message to explain that a > > bit. > > Documentation is okay, but please do it in the code, not the commit > message. That way anyone looking at monitor_get_clock() will be aware > of the constraint. Sure. -- Peter Xu
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 depends on
> qtest_enabled(). After this change, we don't have such a dependency any
> more.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 2504696d76..bd9ab5597f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -282,8 +282,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);
>
> @@ -310,6 +308,15 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
> }
>
> +static inline QEMUClockType monitor_get_clock(void)
> +{
> + if (qtest_enabled()) {
> + return QEMU_CLOCK_VIRTUAL;
> + } else {
> + return QEMU_CLOCK_REALTIME;
> + }
Suggest the more laconic
return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
A comment explaining why we want QEMU_CLOCK_VIRTUAL would be nice.
> +}
> +
> /**
> * Is the current monitor, if any, a QMP monitor?
> */
[...]
On Thu, Jun 07, 2018 at 04:32:54PM +0200, Markus Armbruster wrote:
> 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 depends on
> > qtest_enabled(). After this change, we don't have such a dependency any
> > more.
> >
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > monitor.c | 21 ++++++++++++---------
> > 1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 2504696d76..bd9ab5597f 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -282,8 +282,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);
> >
> > @@ -310,6 +308,15 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> > return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
> > }
> >
> > +static inline QEMUClockType monitor_get_clock(void)
> > +{
> > + if (qtest_enabled()) {
> > + return QEMU_CLOCK_VIRTUAL;
> > + } else {
> > + return QEMU_CLOCK_REALTIME;
> > + }
>
> Suggest the more laconic
>
> return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
>
> A comment explaining why we want QEMU_CLOCK_VIRTUAL would be nice.
Will do.
--
Peter Xu
© 2016 - 2025 Red Hat, Inc.