Re: [PATCH v3 23/34] qapi: Simplify how qmp_dispatch() gets the request ID

[Marc-André Lureau]

Hi

On Tue, Mar 17, 2020 at 7:40 AM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> writes:
>
> > On Sun, Mar 15, 2020 at 3:51 PM Markus Armbruster <address@hidden> wrote:
> >>
> >> We convert the request object to a QDict twice: first in
> >> qmp_dispatch() to get the request ID, and then again in
> >> qmp_dispatch_check_obj(), which converts to QDict, then checks and
> >> returns it.  We can't get the request ID from the latter, because it's
> >> null when the qdict flunks the checks.
> >>
> >> Move getting the request ID into qmp_dispatch_check_obj().
> >>
> >
> > I don't see this is a an improvement. qmp_dispatch_check_obj() doesn't
> > care about id.
> >
> > And it doesn't look like it is saving cycles either.
> >
> > Is that worth it?
> >
> > Code change is ok otherwise,
>
> The duplicated conversion to QDict annoys me, mostly because both copies
> can fail.
>
> But you're right, my solution is hamfisted.  What about this one?
>
>
> From 46a1719be9503f86636ff672325c5430d4063b8b Mon Sep 17 00:00:00 2001
> From: Markus Armbruster <address@hidden>
> Date: Mon, 21 Oct 2019 15:52:20 +0200
> Subject: [PATCH] qapi: Simplify how qmp_dispatch() gets the request ID
>
> We convert the request object to a QDict twice: first in
> qmp_dispatch() to get the request ID, and then again in
> qmp_dispatch_check_obj(), which converts to QDict, then checks and
> returns it.  We can't get the request ID from the latter, because it's
> null when the qdict flunks the checks.
>
> Move the checked conversion to QDict from qmp_dispatch_check_obj() to
> qmp_dispatch(), and drop the duplicate there.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  qapi/qmp-dispatch.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 550d1fe8d2..91e50fa0dd 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -19,20 +19,13 @@
>  #include "sysemu/runstate.h"
>  #include "qapi/qmp/qbool.h"
>
> -static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
> +static QDict *qmp_dispatch_check_obj(QDict *dict, bool allow_oob,
>                                       Error **errp)
>  {
>      const char *exec_key = NULL;
>      const QDictEntry *ent;
>      const char *arg_name;
>      const QObject *arg_obj;
> -    QDict *dict;
> -
> -    dict = qobject_to(QDict, request);
> -    if (!dict) {
> -        error_setg(errp, "QMP input must be a JSON object");
> -        return NULL;
> -    }
>
>      for (ent = qdict_first(dict); ent;
>           ent = qdict_next(dict, ent)) {
> @@ -103,13 +96,21 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject 
> *request,
>      const char *command;
>      QDict *args;
>      QmpCommand *cmd;
> -    QDict *dict = qobject_to(QDict, request);
> -    QObject *id = dict ? qdict_get(dict, "id") : NULL;
> +    QDict *dict;
> +    QObject *id;
>      QObject *ret = NULL;
>      QDict *rsp = NULL;
>
> -    dict = qmp_dispatch_check_obj(request, allow_oob, &err);
> +    dict = qobject_to(QDict, request);
>      if (!dict) {
> +        id = NULL;
> +        error_setg(&err, "QMP input must be a JSON object");
> +        goto out;
> +    }
> +
> +    id = qdict_get(dict, "id");
> +
> +    if (!qmp_dispatch_check_obj(dict, allow_oob, &err)) {
>          goto out;
>      }
>
> --
> 2.21.1
>

It seems cleaner to me,
Reviewed-by: Marc-André Lureau <address@hidden>




-- 
Marc-André Lureau