Hi
On Tue, Mar 17, 2020 at 7:40 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lur...@gmail.com> writes:
>
> > On Sun, Mar 15, 2020 at 3:51 PM Markus Armbruster <arm...@redhat.com> 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 <arm...@redhat.com>
> 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 <arm...@redhat.com>
> ---
> 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 <marcandre.lur...@redhat.com>
--
Marc-André Lureau
