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

[Peter Xu <peterx@redhat.com>]

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