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
