Re: [Qemu-devel] [PATCH v2 18/18] RFC: qmp: common 'id' handling & make QGA conform to QMP spec

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Let qmp_dispatch() copy the 'id' field. That way any qmp client will
> conform to the specification, including QGA. Furthermore, it
> simplifies the work for qemu monitor.
>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c           | 30 +++++++++++-------------------
>  qapi/qmp-dispatch.c | 12 +++++++++---
>  tests/test-qga.c    | 13 +++++--------
>  3 files changed, 25 insertions(+), 30 deletions(-)

I support adding this feature to QGA.

Needs Mike Roth's Ack or R-by.

> diff --git a/monitor.c b/monitor.c
> index 5a41a230b9..11249e7018 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -249,8 +249,6 @@ QEMUBH *qmp_dispatcher_bh;
>  struct QMPRequest {
>      /* Owner of the request */
>      Monitor *mon;
> -    /* "id" field of the request */
> -    QObject *id;
>      /*
>       * Request object to be handled or Error to be reported
>       * (exactly one of them is non-null)
> @@ -351,7 +349,6 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>  
>  static void qmp_request_free(QMPRequest *req)
>  {
> -    qobject_unref(req->id);
>      qobject_unref(req->req);
>      error_free(req->err);
>      g_free(req);
> @@ -3991,18 +3988,14 @@ static int monitor_can_read(void *opaque)
>   * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP.
>   * Nothing is emitted then.
>   */
> -static void monitor_qmp_respond(Monitor *mon, QDict *rsp, QObject *id)
> +static void monitor_qmp_respond(Monitor *mon, QDict *rsp)
>  {
>      if (rsp) {
> -        if (id) {
> -            qdict_put_obj(rsp, "id", qobject_ref(id));
> -        }
> -
>          qmp_send_response(mon, rsp);
>      }
>  }
>  
> -static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
> +static void monitor_qmp_dispatch(Monitor *mon, QObject *req)
>  {
>      Monitor *old_mon;
>      QDict *rsp;
> @@ -4027,7 +4020,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
>          }
>      }
>  
> -    monitor_qmp_respond(mon, rsp, id);
> +    monitor_qmp_respond(mon, rsp);
>      qobject_unref(rsp);
>  }
>  
> @@ -4082,13 +4075,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
>      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);
> +        QDict *qdict = qobject_to(QDict, req_obj->req);
> +        QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
> +        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
> +        monitor_qmp_dispatch(req_obj->mon, req_obj->req);
>      } else {
>          assert(req_obj->err);
>          rsp = qmp_error_response(req_obj->err);
>          req_obj->err = NULL;
> -        monitor_qmp_respond(req_obj->mon, rsp, NULL);
> +        monitor_qmp_respond(req_obj->mon, rsp);
>          qobject_unref(rsp);
>      }
>  
> @@ -4117,8 +4112,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>  
>      qdict = qobject_to(QDict, req);
>      if (qdict) {
> -        id = qobject_ref(qdict_get(qdict, "id"));
> -        qdict_del(qdict, "id");
> +        id = qdict_get(qdict, "id");

Write of @id.

>      } /* else will fail qmp_dispatch() */
>  
>      if (req && trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
           QString *req_json = qobject_to_json(req);
           trace_handle_qmp_command(mon, qstring_get_str(req_json));
           qobject_unref(req_json);
       }
> @@ -4129,15 +4123,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>  
>      if (qdict && qmp_is_oob(qdict)) {
>          /* OOB commands are executed immediately */
> -        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id)
> -                                          ?: "");
> -        monitor_qmp_dispatch(mon, req, id);
> +        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: "");
> +        monitor_qmp_dispatch(mon, req);

First use of @id.

>      }
>  
>      req_obj = g_new0(QMPRequest, 1);
>      req_obj->mon = mon;
> -    req_obj->id = id;
>      req_obj->req = req;
>      req_obj->err = err;
>  
       /* Protect qmp_requests and fetching its length. */
       qemu_mutex_lock(&mon->qmp.qmp_queue_lock);

       /*
        * If OOB is not enabled on the current monitor, we'll emulate the
        * old behavior that we won't process the current monitor any more
        * until it has responded.  This helps make sure that as long as
        * OOB is not enabled, the server will never drop any command.
        */
       if (!qmp_oob_enabled(mon)) {
           monitor_suspend(mon);
       } else {
           /* Drop the request if queue is full. */
           if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
               qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
               /*
                * FIXME @id's scope is just @mon, and broadcasting it is
                * wrong.  If another monitor's client has a command with
                * the same ID in flight, the event will incorrectly claim
                * that command was dropped.
                */
               qapi_event_send_command_dropped(id,
                                               COMMAND_DROP_REASON_QUEUE_FULL,
                                               &error_abort);
               qmp_request_free(req_obj);
               return;

Second use of @id, will go away when we get rid of flawed event
COMMAND_DROPPED.

           }
       }

       /*
        * Put the request to the end of queue so that requests will be
        * handled in time order.  Ownership for req_obj, req, id,

Comment needs an update: @id is no longer transferred.

        * etc. will be delivered to the handler side.
        */
       g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
       qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);

       /* Kick the dispatcher routine */
       qemu_bh_schedule(qmp_dispatcher_bh);
   }

Once COMMAND_DROPPED is gone, the assignment to @id should go next to
its only remaining use.

Perhaps we should remove it before this patch to reduce churn.

> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 90ba5e3074..e714cfb8ff 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -59,6 +59,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
>                             "QMP input member 'arguments' must be an object");
>                  return NULL;
>              }
> +        } else if (!strcmp(arg_name, "id")) {
> +            continue;
>          } else {
>              error_setg(errp, "QMP input member '%s' is unexpected",
>                         arg_name);
> @@ -166,11 +168,11 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
>                      bool allow_oob)
>  {
>      Error *err = NULL;
> -    QObject *ret;
> +    QDict *dict = qobject_to(QDict, request);
> +    QObject *id = dict ? qdict_get(dict, "id") : NULL;
> +    QObject *ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
>      QDict *rsp;
>  
> -    ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
> -
>      if (err) {

This separates the call from its error check.  I don't like that.
Recommend:

       QObject *ret;
  +    QDict *dict = qobject_to(QDict, request);
  +    QObject *id = dict ? qdict_get(dict, "id") : NULL;
       QDict *rsp;
   
       ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
  -
       if (err) {

>          rsp = qmp_error_response(err);
>      } else if (ret) {
> @@ -181,5 +183,9 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
>          rsp = NULL;
>      }
>  
> +    if (rsp && id) {
> +        qdict_put_obj(rsp, "id", qobject_ref(id));
> +    }
> +
>      return rsp;
>  }
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index d638b1571a..4ee8b405f4 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -227,18 +227,15 @@ static void test_qga_ping(gconstpointer fix)
>      qobject_unref(ret);
>  }
>  
> -static void test_qga_invalid_id(gconstpointer fix)
> +static void test_qga_id(gconstpointer fix)
>  {
>      const TestFixture *fixture = fix;
> -    QDict *ret, *error;
> -    const char *class;
> +    QDict *ret;
>  
>      ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
>      g_assert_nonnull(ret);
> -
> -    error = qdict_get_qdict(ret, "error");
> -    class = qdict_get_try_str(error, "class");
> -    g_assert_cmpstr(class, ==, "GenericError");
> +    qmp_assert_no_error(ret);
> +    g_assert_cmpint(qdict_get_int(ret, "id"), ==, 1);
>  
>      qobject_unref(ret);
>  }
> @@ -1014,7 +1011,7 @@ int main(int argc, char **argv)
>      g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops);
>      g_test_add_data_func("/qga/file-write-read", &fix, test_qga_file_write_read);
>      g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time);
> -    g_test_add_data_func("/qga/invalid-id", &fix, test_qga_invalid_id);
> +    g_test_add_data_func("/qga/id", &fix, test_qga_id);
>      g_test_add_data_func("/qga/invalid-oob", &fix, test_qga_invalid_oob);
>      g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd);
>      g_test_add_data_func("/qga/invalid-args", &fix, test_qga_invalid_args);

I thought "doc update missing", but then I checked qmp-spec.txt, and
it doesn't mention "id" works only for QEMU. not for QGA.  And then I
realized you pointed that out in your commit message.

With the comment updated:
Reviewed-by: Markus Armbruster <armbru@redhat.com>