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().
signature.asc
Description: PGP signature
