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

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> ---
>  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);
>      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...

>          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 <armbru@redhat.com>