All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error
@ 2016-09-30  9:59 Marc-André Lureau
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 1/3] qapi: add assert about root value Marc-André Lureau
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Marc-André Lureau @ 2016-09-30  9:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau

Hi,

'monitor: use qmp_dispatch()' patch broke some iotests expecting a
'missing parameter' error. This series fixes qapi visitors to return
this error for all types.

This series should go on top of:
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05949.html

v3:
- merge patch 4 (iotest fix) with patch 3
- add r-b tags

v2:
- add some assert() for expected values in qmp-input-visitor, making
  it more explicit when qmp_input_get_object() may return NULL
- squash the *obj = NULL changes with the missing arg error patch
- move qobject_to_*(qobj) calls after checking qobj != NULL
- update commit messages

Marc-André Lureau (3):
  qapi: add assert about root value
  qapi: assert list entry has a value
  qapi: return a 'missing parameter' error

 qapi/qmp-input-visitor.c   | 72 +++++++++++++++++++++++++++++++++++-----------
 tests/qemu-iotests/087.out |  2 +-
 2 files changed, 56 insertions(+), 18 deletions(-)

-- 
2.10.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v3 1/3] qapi: add assert about root value
  2016-09-30  9:59 [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error Marc-André Lureau
@ 2016-09-30  9:59 ` Marc-André Lureau
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 2/3] qapi: assert list entry has a value Marc-André Lureau
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2016-09-30  9:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau

qiv->root should not be null, make that clearer with some assert.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qmp-input-visitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index fc91e74..c7deca9 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -64,6 +64,7 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
 
     if (QSLIST_EMPTY(&qiv->stack)) {
         /* Starting at root, name is ignored. */
+        assert(qiv->root);
         return qiv->root;
     }
 
@@ -395,6 +396,7 @@ Visitor *qmp_input_visitor_new(QObject *obj, bool strict)
 {
     QmpInputVisitor *v;
 
+    assert(obj);
     v = g_malloc0(sizeof(*v));
 
     v->visitor.type = VISITOR_INPUT;
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v3 2/3] qapi: assert list entry has a value
  2016-09-30  9:59 [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error Marc-André Lureau
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 1/3] qapi: add assert about root value Marc-André Lureau
@ 2016-09-30  9:59 ` Marc-André Lureau
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error Marc-André Lureau
  2016-10-06 13:26 ` [Qemu-devel] [PATCH v3 0/3] qapi: return " Markus Armbruster
  3 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2016-09-30  9:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau

This helps to figure out the expectations.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qmp-input-visitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index c7deca9..fe097c9 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -84,6 +84,7 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
         assert(qobject_type(qobj) == QTYPE_QLIST);
         assert(!name);
         ret = qlist_entry_obj(tos->entry);
+        assert(ret);
         if (consume) {
             tos->entry = qlist_next(tos->entry);
         }
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error
  2016-09-30  9:59 [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error Marc-André Lureau
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 1/3] qapi: add assert about root value Marc-André Lureau
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 2/3] qapi: assert list entry has a value Marc-André Lureau
@ 2016-09-30  9:59 ` Marc-André Lureau
  2016-10-05 13:40   ` Markus Armbruster
  2016-10-06 13:26 ` [Qemu-devel] [PATCH v3 0/3] qapi: return " Markus Armbruster
  3 siblings, 1 reply; 8+ messages in thread
From: Marc-André Lureau @ 2016-09-30  9:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau

The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing
parameters, but the qapi qmp_dispatch() code uses
QERR_INVALID_PARAMETER_TYPE.

Improve qapi code to return QERR_MISSING_PARAMETER where
appropriate.

Fix expected error message in iotests.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qmp-input-visitor.c   | 69 ++++++++++++++++++++++++++++++++++------------
 tests/qemu-iotests/087.out |  2 +-
 2 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index fe097c9..9b9b350 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -56,7 +56,7 @@ static QmpInputVisitor *to_qiv(Visitor *v)
 
 static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
                                      const char *name,
-                                     bool consume)
+                                     bool consume, Error **errp)
 {
     StackObject *tos;
     QObject *qobj;
@@ -80,6 +80,9 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
             bool removed = g_hash_table_remove(tos->h, name);
             assert(removed);
         }
+        if (!ret) {
+            error_setg(errp, QERR_MISSING_PARAMETER, name);
+        }
     } else {
         assert(qobject_type(qobj) == QTYPE_QLIST);
         assert(!name);
@@ -165,13 +168,16 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
                                    size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qmp_input_get_object(qiv, name, true);
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
     Error *err = NULL;
 
     if (obj) {
         *obj = NULL;
     }
-    if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
+    if (!qobj) {
+        return;
+    }
+    if (qobject_type(qobj) != QTYPE_QDICT) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "QDict");
         return;
@@ -193,10 +199,13 @@ static void qmp_input_start_list(Visitor *v, const char *name,
                                  GenericList **list, size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qmp_input_get_object(qiv, name, true);
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
     const QListEntry *entry;
 
-    if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
+    if (!qobj) {
+        return;
+    }
+    if (qobject_type(qobj) != QTYPE_QLIST) {
         if (list) {
             *list = NULL;
         }
@@ -234,11 +243,10 @@ static void qmp_input_start_alternate(Visitor *v, const char *name,
                                       bool promote_int, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qmp_input_get_object(qiv, name, false);
+    QObject *qobj = qmp_input_get_object(qiv, name, false, errp);
 
     if (!qobj) {
         *obj = NULL;
-        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
         return;
     }
     *obj = g_malloc0(size);
@@ -252,8 +260,13 @@ static void qmp_input_type_int64(Visitor *v, const char *name, int64_t *obj,
                                  Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
+    QInt *qint;
 
+    if (!qobj) {
+        return;
+    }
+    qint = qobject_to_qint(qobj);
     if (!qint) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "integer");
@@ -268,8 +281,13 @@ static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj,
 {
     /* FIXME: qobject_to_qint mishandles values over INT64_MAX */
     QmpInputVisitor *qiv = to_qiv(v);
-    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
+    QInt *qint;
 
+    if (!qobj) {
+        return;
+    }
+    qint = qobject_to_qint(qobj);
     if (!qint) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "integer");
@@ -283,8 +301,13 @@ static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj,
                                 Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true));
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
+    QBool *qbool;
 
+    if (!qobj) {
+        return;
+    }
+    qbool = qobject_to_qbool(qobj);
     if (!qbool) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "boolean");
@@ -298,10 +321,15 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj,
                                Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
+    QString *qstr;
 
+    *obj = NULL;
+    if (!qobj) {
+        return;
+    }
+    qstr = qobject_to_qstring(qobj);
     if (!qstr) {
-        *obj = NULL;
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "string");
         return;
@@ -314,10 +342,13 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj,
                                   Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qmp_input_get_object(qiv, name, true);
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
     QInt *qint;
     QFloat *qfloat;
 
+    if (!qobj) {
+        return;
+    }
     qint = qobject_to_qint(qobj);
     if (qint) {
         *obj = qint_get_int(qobject_to_qint(qobj));
@@ -338,11 +369,15 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
                                Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qmp_input_get_object(qiv, name, true);
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
+
+    *obj = NULL;
+    if (!qobj) {
+        return;
+    }
 
     if (!qobj) {
         error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
-        *obj = NULL;
         return;
     }
 
@@ -353,7 +388,7 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
 static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qmp_input_get_object(qiv, name, true);
+    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
 
     if (!qobj) {
         error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
@@ -369,7 +404,7 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
 static void qmp_input_optional(Visitor *v, const char *name, bool *present)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qmp_input_get_object(qiv, name, false);
+    QObject *qobj = qmp_input_get_object(qiv, name, false, NULL);
 
     if (!qobj) {
         *present = false;
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index bef6862..55ba6a7 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -56,7 +56,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
 Testing: -S
 QMP_VERSION
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'driver', expected: string"}}
+{"error": {"class": "GenericError", "desc": "Parameter 'driver' is missing"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
 
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error Marc-André Lureau
@ 2016-10-05 13:40   ` Markus Armbruster
  2016-10-05 14:40     ` Markus Armbruster
  0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2016-10-05 13:40 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, berto

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

> The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing
> parameters, but the qapi qmp_dispatch() code uses
> QERR_INVALID_PARAMETER_TYPE.
>
> Improve qapi code to return QERR_MISSING_PARAMETER where
> appropriate.
>
> Fix expected error message in iotests.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/qmp-input-visitor.c   | 69 ++++++++++++++++++++++++++++++++++------------
>  tests/qemu-iotests/087.out |  2 +-
>  2 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index fe097c9..9b9b350 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -56,7 +56,7 @@ static QmpInputVisitor *to_qiv(Visitor *v)
>  
>  static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>                                       const char *name,
> -                                     bool consume)
> +                                     bool consume, Error **errp)
>  {
>      StackObject *tos;
>      QObject *qobj;
> @@ -80,6 +80,9 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>              bool removed = g_hash_table_remove(tos->h, name);
>              assert(removed);
>          }
> +        if (!ret) {
> +            error_setg(errp, QERR_MISSING_PARAMETER, name);
> +        }
>      } else {
>          assert(qobject_type(qobj) == QTYPE_QLIST);
>          assert(!name);

Note for later: qmp_input_get_object() either returns non-null and
leaves @errp alone, or it returns null and sets an error.

> @@ -165,13 +168,16 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
>                                     size_t size, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
>      Error *err = NULL;
>  
>      if (obj) {
>          *obj = NULL;
>      }
> -    if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
> +    if (!qobj) {
> +        return;
> +    }
> +    if (qobject_type(qobj) != QTYPE_QDICT) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "QDict");
>          return;
> @@ -193,10 +199,13 @@ static void qmp_input_start_list(Visitor *v, const char *name,
>                                   GenericList **list, size_t size, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
>      const QListEntry *entry;
>  
> -    if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
> +    if (!qobj) {
> +        return;
> +    }
> +    if (qobject_type(qobj) != QTYPE_QLIST) {
>          if (list) {
>              *list = NULL;
>          }
> @@ -234,11 +243,10 @@ static void qmp_input_start_alternate(Visitor *v, const char *name,
>                                        bool promote_int, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qmp_input_get_object(qiv, name, false);
> +    QObject *qobj = qmp_input_get_object(qiv, name, false, errp);
>  
>      if (!qobj) {
>          *obj = NULL;
> -        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
>          return;
>      }
>      *obj = g_malloc0(size);
> @@ -252,8 +260,13 @@ static void qmp_input_type_int64(Visitor *v, const char *name, int64_t *obj,
>                                   Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
> +    QInt *qint;
>  
> +    if (!qobj) {
> +        return;
> +    }
> +    qint = qobject_to_qint(qobj);
>      if (!qint) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "integer");
> @@ -268,8 +281,13 @@ static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>  {
>      /* FIXME: qobject_to_qint mishandles values over INT64_MAX */
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
> +    QInt *qint;
>  
> +    if (!qobj) {
> +        return;
> +    }
> +    qint = qobject_to_qint(qobj);
>      if (!qint) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "integer");
> @@ -283,8 +301,13 @@ static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj,
>                                  Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true));
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
> +    QBool *qbool;
>  
> +    if (!qobj) {
> +        return;
> +    }
> +    qbool = qobject_to_qbool(qobj);
>      if (!qbool) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "boolean");
> @@ -298,10 +321,15 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj,
>                                 Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
> +    QString *qstr;
>  
> +    *obj = NULL;
> +    if (!qobj) {
> +        return;
> +    }
> +    qstr = qobject_to_qstring(qobj);
>      if (!qstr) {
> -        *obj = NULL;
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "string");
>          return;
> @@ -314,10 +342,13 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj,
>                                    Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
>      QInt *qint;
>      QFloat *qfloat;
>  
> +    if (!qobj) {
> +        return;
> +    }
>      qint = qobject_to_qint(qobj);
>      if (qint) {
>          *obj = qint_get_int(qobject_to_qint(qobj));
> @@ -338,11 +369,15 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
>                                 Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
> +
> +    *obj = NULL;
> +    if (!qobj) {
> +        return;
> +    }
>  
>      if (!qobj) {
>          error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> -        *obj = NULL;
>          return;
>      }

The second conditional is dead.  Mismerge?

The obvious fix is to drop it.  Can do on commit.

>  
> @@ -353,7 +388,7 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
>  static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
>  
>      if (!qobj) {
>          error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
           return;

When qmp_input_get_object() returns null, it also sets an error.  The
conditional then sets an error again, which is wrong and can fail the
assertion in error_setv().

The obvious fix is to drop the error_setg().  Can do on commit.

       }

       if (qobject_type(qobj) != QTYPE_QNULL) {
           error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                      "null");
       }
   }

> @@ -369,7 +404,7 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
>  static void qmp_input_optional(Visitor *v, const char *name, bool *present)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QObject *qobj = qmp_input_get_object(qiv, name, false);
> +    QObject *qobj = qmp_input_get_object(qiv, name, false, NULL);
>  
>      if (!qobj) {
>          *present = false;
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> index bef6862..55ba6a7 100644
> --- a/tests/qemu-iotests/087.out
> +++ b/tests/qemu-iotests/087.out
> @@ -56,7 +56,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
>  Testing: -S
>  QMP_VERSION
>  {"return": {}}
> -{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'driver', expected: string"}}
> +{"error": {"class": "GenericError", "desc": "Parameter 'driver' is missing"}}
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error
  2016-10-05 13:40   ` Markus Armbruster
@ 2016-10-05 14:40     ` Markus Armbruster
  0 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2016-10-05 14:40 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, berto

The appended fixups are necessary to pass the tests from "[PATCH 2.5/3]
tests/test-qmp-input-strict: Cover missing struct members".  Can squash
in on commit.

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 9b9b350..37a8e1f 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -376,11 +376,6 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
         return;
     }
 
-    if (!qobj) {
-        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
-        return;
-    }
-
     qobject_incref(qobj);
     *obj = qobj;
 }
@@ -391,7 +386,6 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
     QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
 
     if (!qobj) {
-        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
         return;
     }
 

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error
  2016-09-30  9:59 [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error Marc-André Lureau
                   ` (2 preceding siblings ...)
  2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error Marc-André Lureau
@ 2016-10-06 13:26 ` Markus Armbruster
  2016-10-06 13:53   ` Marc-André Lureau
  3 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2016-10-06 13:26 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, berto

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

> Hi,
>
> 'monitor: use qmp_dispatch()' patch broke some iotests expecting a
> 'missing parameter' error. This series fixes qapi visitors to return
> this error for all types.
>
> This series should go on top of:
> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05949.html

Touched up as discussed, and applied to my qapi-next branch.  Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error
  2016-10-06 13:26 ` [Qemu-devel] [PATCH v3 0/3] qapi: return " Markus Armbruster
@ 2016-10-06 13:53   ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2016-10-06 13:53 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: berto, qemu-devel

Hi

On Thu, Oct 6, 2016 at 5:48 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi,
> >
> > 'monitor: use qmp_dispatch()' patch broke some iotests expecting a
> > 'missing parameter' error. This series fixes qapi visitors to return
> > this error for all types.
> >
> > This series should go on top of:
> > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05949.html
>
> Touched up as discussed, and applied to my qapi-next branch.  Thanks!
>

thanks Markus!
-- 
Marc-André Lureau

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-10-06 13:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-30  9:59 [Qemu-devel] [PATCH v3 0/3] qapi: return 'missing parameter' error Marc-André Lureau
2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 1/3] qapi: add assert about root value Marc-André Lureau
2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 2/3] qapi: assert list entry has a value Marc-André Lureau
2016-09-30  9:59 ` [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error Marc-André Lureau
2016-10-05 13:40   ` Markus Armbruster
2016-10-05 14:40     ` Markus Armbruster
2016-10-06 13:26 ` [Qemu-devel] [PATCH v3 0/3] qapi: return " Markus Armbruster
2016-10-06 13:53   ` Marc-André Lureau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.