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

[Peter Xu <peterx@redhat.com>]

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