[Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error

[Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error

[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.

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 (4):
  qapi: add assert about root value
  qapi: assert list entry has a value
  qapi: return a 'missing parameter' error
  iotests: fix expected error message

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

-- 
2.10.0

[Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value

[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>
---
 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 64dd392..be52aaf 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;
     }
 
@@ -384,6 +385,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

[Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value

[Marc-André Lureau]

This helps to figure out the expectations.

Signed-off-by: Marc-André Lureau <marcandre.lureau@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 be52aaf..c1019d6 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

[Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error

[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_INVALID_PARAMETER_TYPE where
appropriate.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 17 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index c1019d6..6f85664 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);
 
+    *obj = NULL;
     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,7 +369,12 @@ 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;
+    }
 
     qobject_incref(qobj);
     *obj = qobj;
@@ -347,8 +383,11 @@ 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) {
+        return;
+    }
     if (qobject_type(qobj) != QTYPE_QNULL) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "null");
@@ -358,7 +397,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;
-- 
2.10.0

[Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message

[Marc-André Lureau]

Missing argument returns a corresponding error message for all types
now.

The "old" qmp dispatch code didn't return QERR_MISSING_PARAMETER for
argument structure members (it returned QERR_INVALID_PARAMETER). Only
for top-level argument it did return QERR_MISSING_PARAMETER. It is
preferable to have a consistent error for missing fields in inner
structs as well.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/087.out | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index a95c4b0..b213db2 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -60,7 +60,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

Re: [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On 09/21/2016 03:10 PM, Marc-André Lureau wrote:
> qiv->root should not be null, make that clearer with some assert.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/qmp-input-visitor.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

On 09/21/2016 03:10 PM, Marc-André Lureau wrote:
> This helps to figure out the expectations.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/qmp-input-visitor.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index be52aaf..c1019d6 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);
>          }
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On 09/21/2016 03:10 PM, Marc-André Lureau wrote:
> Missing argument returns a corresponding error message for all types
> now.
> 
> The "old" qmp dispatch code didn't return QERR_MISSING_PARAMETER for
> argument structure members (it returned QERR_INVALID_PARAMETER). Only
> for top-level argument it did return QERR_MISSING_PARAMETER. It is
> preferable to have a consistent error for missing fields in inner
> structs as well.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/087.out | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Yes, this looks better.

If the message is changed earlier in this series, then this should be
squashed into that patch (3/4?); if the message changed as the result of
an earlier commit, the commit message should call out the id at which
the tree was temporarily broken.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On 09/21/2016 03:10 PM, Marc-André Lureau wrote:
> 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_INVALID_PARAMETER_TYPE where
> appropriate.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 17 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message

[Marc-André Lureau]

----- Original Message -----
> On 09/21/2016 03:10 PM, Marc-André Lureau wrote:
> > Missing argument returns a corresponding error message for all types
> > now.
> > 
> > The "old" qmp dispatch code didn't return QERR_MISSING_PARAMETER for
> > argument structure members (it returned QERR_INVALID_PARAMETER). Only
> > for top-level argument it did return QERR_MISSING_PARAMETER. It is
> > preferable to have a consistent error for missing fields in inner
> > structs as well.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> > ---
> >  tests/qemu-iotests/087.out | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Yes, this looks better.
> 
> If the message is changed earlier in this series, then this should be
> squashed into that patch (3/4?); if the message changed as the result of
> an earlier commit, the commit message should call out the id at which
> the tree was temporarily broken.

I checked, it's the previous patch that introduce the change (I wasn't sure given that in alternate case qapi already returned MISSING_PARAMETER), it can be squashed with it.

Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error

[Daniel P. Berrange]

On Thu, Sep 22, 2016 at 12:10:17AM +0400, Marc-André Lureau wrote:
> 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_INVALID_PARAMETER_TYPE where
> appropriate.

Surely this is meant to say 'return QERR_MISSING_PARAMETER', as
QERR_INVALID_PARAMETER_TYPE is what its already returning.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 17 deletions(-)

There's a pretty detailed unit test for this visitor, which
should be further extended to cover this new behaviour.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error

[Markus Armbruster]

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_INVALID_PARAMETER_TYPE where
> appropriate.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index c1019d6..6f85664 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);
>  
> +    *obj = NULL;
>      if (!qobj) {
> -        *obj = NULL;
> -        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
>          return;
>      }
>      *obj = g_malloc0(size);

The patch does more than one thing: in addition to fixing the 'missing
parameter' regression, it also messes with *obj = NULL in places.  These
changes may well make sense, but they should be a separate patch, to
ease review.

> @@ -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,7 +369,12 @@ 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;
> +    }
>  
>      qobject_incref(qobj);
>      *obj = qobj;

The patch does a third thing: it fixes a crash bug.  The old code fails
to fail when the parameter doesn't exist.  Instead, it sets *obj = NULL,
violating its contract.  Reproducer:

    { "execute": "qom-set",
      "arguments": { "path": "/machine", "property": "rtc-time" } }

Separate patch, please, cc: qemu-stable.

> @@ -347,8 +383,11 @@ 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) {
> +        return;
> +    }
>      if (qobject_type(qobj) != QTYPE_QNULL) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "null");

Same bug, I think, but I don't have a reproducer handy.

> @@ -358,7 +397,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;

Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error

[Marc-André Lureau]

Hi

----- Original Message -----
> 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_INVALID_PARAMETER_TYPE where
> > appropriate.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> > ---
> >  qapi/qmp-input-visitor.c | 73
> >  +++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 56 insertions(+), 17 deletions(-)
> >
> > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > index c1019d6..6f85664 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);
> >  
> > +    *obj = NULL;
> >      if (!qobj) {
> > -        *obj = NULL;
> > -        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> >          return;
> >      }
> >      *obj = g_malloc0(size);
> 
> The patch does more than one thing: in addition to fixing the 'missing
> parameter' regression, it also messes with *obj = NULL in places.  These
> changes may well make sense, but they should be a separate patch, to
> ease review.

Looking more at it, I think we can leave it only in the error case.

> 
> > @@ -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,7 +369,12 @@ 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;
> > +    }
> >  
> >      qobject_incref(qobj);
> >      *obj = qobj;
> 
> The patch does a third thing: it fixes a crash bug.  The old code fails
> to fail when the parameter doesn't exist.  Instead, it sets *obj = NULL,
> violating its contract.  Reproducer:
> 
>     { "execute": "qom-set",
>       "arguments": { "path": "/machine", "property": "rtc-time" } }
> 
> Separate patch, please, cc: qemu-stable.
> 

We can make it a seperate patch, but we still need to return an error, that's what this patch does.

A seperate patch will look like:

@@ -338,6 +338,12 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, true);
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        *obj = NULL;
+        return;
+    }

I think we may as well just take this patch, what do you think?


> > @@ -347,8 +383,11 @@ 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) {
> > +        return;
> > +    }
> >      if (qobject_type(qobj) != QTYPE_QNULL) {
> >          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name :
> >          "null",
> >                     "null");
> 
> Same bug, I think, but I don't have a reproducer handy.
> 
> > @@ -358,7 +397,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;
>