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

[Markus Armbruster]

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