Re: [Qemu-devel] [PATCH 05/12] monitor: no need to save need_resume

[Peter Xu]

On Tue, Jul 17, 2018 at 07:38:32AM +0200, Markus Armbruster wrote:
> Marc-André Lureau <address@hidden> writes:
> 
> > There is no need for per-command need_resume granularity, it should
> > resume after running an non-oob command on oob-disabled monitor.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  monitor.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 462cf96f7b..ec40a80d81 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -257,12 +257,6 @@ struct QMPRequest {
> >       */
> >      QObject *req;
> >      Error *err;
> > -    /*
> > -     * Whether we need to resume the monitor afterward.  This flag is
> > -     * used to emulate the old QMP server behavior that the current
> > -     * command must be completed before execution of the next one.
> > -     */
> > -    bool need_resume;
> 
> Note for later: this comment goes away without a replacement.
> 
> >  };
> >  typedef struct QMPRequest QMPRequest;
> >  
> > @@ -4079,11 +4073,13 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >  {
> >      QMPRequest *req_obj = monitor_qmp_requests_pop_any();
> >      QDict *rsp;
> > +    bool need_resume;
> >  
> >      if (!req_obj) {
> >          return;
> >      }
> >  
> > +    need_resume = !qmp_oob_enabled(req_obj->mon);

[1]

> >      if (req_obj->req) {
> >          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: 
> > "");
> >          monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
> > @@ -4094,7 +4090,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >          qobject_unref(rsp);
> >      }
> >  
> > -    if (req_obj->need_resume) {
> > +    if (need_resume) {
> >          /* Pairs with the monitor_suspend() in handle_qmp_command() */
> 
> Why not
> 
>        if (!qmp_oob_enabled(req_obj->mon)) {
> 
> and drop the variable?  Perhaps you keep the variable to have its name
> provide a hint on why we resume.  But even with that hint and the "pairs
> with" comment, it's still less than clear.  What about...

IMO it's because the value of qmp_oob_enabled(req_obj->mon) might
change along the way, and we need to cache the old result to decide
whether we'll need to resume the monitor.  The trick is that when we
send the first "qmp_capabilities" command to enable out-of-band, then
when at [1] we will get a FALSE (since out-of-band is off by default)
while when reach here we'll get a TRUE instead (after we handled the
"qmp_capabilities" command, out-of-band is enabled).

So if we really want to remove the req_obj->need_resume variable or
anything similar, IMHO we'd better add some comment at [1] mentioning
about this trick.

Thanks,

> 
> >          monitor_resume(req_obj->mon);
> >      }
> > @@ -4146,7 +4142,6 @@ static void handle_qmp_command(JSONMessageParser 
> > *parser, GQueue *tokens)
> >      req_obj->id = id;
> >      req_obj->req = req;
> >      req_obj->err = err;
> > -    req_obj->need_resume = false;
> >  
> >      /* Protect qmp_requests and fetching its length. */
> >      qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > @@ -4159,7 +4154,6 @@ static void handle_qmp_command(JSONMessageParser 
> > *parser, GQueue *tokens)
> >       */
> >      if (!qmp_oob_enabled(mon)) {
> 
> ... adding a replacement for the deleted comment here, say
> 
>            /*
>            * Emulate traditional QMP server behavior: read next command
>            * only after current command completed.  Pairs with
>            * monitor_resume() in monitor_qmp_bh_dispatcher().
>            */
> 
> Would you then be okay with cutting out the variable?
> 
> >          monitor_suspend(mon);
> > -        req_obj->need_resume = true;
> >      } else {
> >          /* Drop the request if queue is full. */
> >          if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> 
> Your patch works fine as far as I can tell, so
> Reviewed-by: Markus Armbruster <address@hidden>

-- 
Peter Xu