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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature