[v3] Configurable policy for handling deprecated interfaces

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

Posted by Markus Armbruster 5 years, 11 months ago

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().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/qmp-dispatch.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 550d1fe8d2..112d29a9ab 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -20,7 +20,7 @@
 #include "qapi/qmp/qbool.h"
 
 static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
-                                     Error **errp)
+                                     QObject **id, Error **errp)
 {
     const char *exec_key = NULL;
     const QDictEntry *ent;
@@ -30,10 +30,13 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
 
     dict = qobject_to(QDict, request);
     if (!dict) {
+        *id = NULL;
         error_setg(errp, "QMP input must be a JSON object");
         return NULL;
     }
 
+    *id = qdict_get(dict, "id");
+
     for (ent = qdict_first(dict); ent;
          ent = qdict_next(dict, ent)) {
         arg_name = qdict_entry_key(ent);
@@ -103,12 +106,12 @@ 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 = qmp_dispatch_check_obj(request, allow_oob, &id, &err);
     if (!dict) {
         goto out;
     }
-- 
2.21.1

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

Posted by Marc-André Lureau 5 years, 10 months ago

On Sun, Mar 15, 2020 at 3:51 PM Markus Armbruster <armbru@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,

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/qmp-dispatch.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 550d1fe8d2..112d29a9ab 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -20,7 +20,7 @@
>  #include "qapi/qmp/qbool.h"
>
>  static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
> -                                     Error **errp)
> +                                     QObject **id, Error **errp)
>  {
>      const char *exec_key = NULL;
>      const QDictEntry *ent;
> @@ -30,10 +30,13 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
>
>      dict = qobject_to(QDict, request);
>      if (!dict) {
> +        *id = NULL;
>          error_setg(errp, "QMP input must be a JSON object");
>          return NULL;
>      }
>
> +    *id = qdict_get(dict, "id");
> +
>      for (ent = qdict_first(dict); ent;
>           ent = qdict_next(dict, ent)) {
>          arg_name = qdict_entry_key(ent);
> @@ -103,12 +106,12 @@ 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 = qmp_dispatch_check_obj(request, allow_oob, &id, &err);
>      if (!dict) {
>          goto out;
>      }
> --
> 2.21.1
>
>


-- 
Marc-André Lureau

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

Posted by Markus Armbruster 5 years, 10 months ago

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

> On Sun, Mar 15, 2020 at 3:51 PM Markus Armbruster <armbru@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 <armbru@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 <armbru@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

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

Posted by Marc-André Lureau 5 years, 10 months ago

Hi

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




-- 
Marc-André Lureau