Re: [Qemu-devel] [RFC v5 16/26] monitor: separate QMP parser and dispatcher

[Peter Xu]

On Mon, Dec 18, 2017 at 09:10:53AM +0000, Stefan Hajnoczi wrote:
> On Mon, Dec 18, 2017 at 01:26:49PM +0800, Peter Xu wrote:
> > On Sat, Dec 16, 2017 at 09:23:22AM +0000, Stefan Hajnoczi wrote:
> > > On Sat, Dec 16, 2017 at 02:37:03PM +0800, Peter Xu wrote:
> > > > On Wed, Dec 13, 2017 at 08:09:38PM +0000, Stefan Hajnoczi wrote:
> > > > > On Tue, Dec 05, 2017 at 01:51:50PM +0800, Peter Xu wrote:
> > > > > > @@ -3956,12 +3968,122 @@ static void 
> > > > > > handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
> > > > > >          }
> > > > > >      }
> > > > > >  
> > > > > > -err_out:
> > > > > > -    monitor_qmp_respond(mon, rsp, err, id);
> > > > > > +    /* Respond if necessary */
> > > > > > +    monitor_qmp_respond(mon, rsp, NULL, id);
> > > > > > +
> > > > > > +    /* This pairs with the monitor_suspend() in 
> > > > > > handle_qmp_command(). */
> > > > > > +    if (!qmp_oob_enabled(mon)) {
> > > > > > +        monitor_resume(mon);
> > > > > 
> > > > > monitor_resume() does not work between threads: if the event loop is
> > > > > currently blocked in poll() it won't notice that the monitor fd should
> > > > > be watched again.
> > > > > 
> > > > > Please add aio_notify() to monitor_resume() and monitor_suspend().  
> > > > > That
> > > > > way the event loop is forced to check can_read() again.
> > > > 
> > > > Ah, yes.  I think monitor_suspend() does not need the notify?  Since
> > > > if it's sleeping it won't miss the next check in can_read() after all?
> > > 
> > > No, that would be a bug.  Imagine the IOThread is blocked in poll
> > > monitoring the chardev file descriptor when the main loop calls
> > > monitor_suspend().  If the file descriptors becomes readable then the
> > > handler function executes even though the monitor is supposed to be
> > > suspended!
> > 
> > When you say "the handler function executes", do you mean the handler
> > that has already added to the qmp request queue, or the one that
> > hasn't yet parsed by the parser?
> 
> The chardev file descriptor handler function (the QMP parser).
> 
> > For the previous case (the handler that has queued already): IMHO
> > that's what we expect it to behave, say, when we call
> > monitor_suspend(), we only stop accepting and parsing new inputs from
> > the user, but the requests on the queue should still be processed.
> > 
> > For the latter (the handler of a newly typed command):
> > monitor_suspend() should suspend the parser already, so
> > monitor_can_read() check should fail, then that command should never
> > be queued until we call another monitor_resume().
> 
> You are assuming that monitor_can_read() is called *after* poll()
> returns.  This is what I tried to explain in the previous reply.
> 
> The the monitor_can_read() function is called *before* the blocking
> poll() syscall.  If something changes the monitor_can_read() return
> value, you must* kick the event loop to ensure that the event loop
> reflects this change.
> 
> If you want to check how this works, see chardev/char-io.c for how
> fd_can_read() is used.
> 
> * Currently monitor.c doesn't need to kick the event loop explicitly
> because it runs within the main loop thread.  Therefore the event loop
> always calls monitor_can_read() again before entering poll().

Yeah I got it.  I was assuming the check is done in check() of the
watcher, but obviously I was wrong. :(

Thanks for clarifying this.  I'll do proper kicking.

-- 
Peter Xu