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

[Qemu-devel] [PATCH 0/3] 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.

Marc-André Lureau (3):
  qapi: return a 'missing parameter' error
  qapi: clear given pointer
  iotests: fix expected error message

 qapi/qmp-input-visitor.c   | 116 ++++++++++++++++++++++++++++++---------------
 tests/qemu-iotests/087.out |   2 +-
 2 files changed, 80 insertions(+), 38 deletions(-)

-- 
2.10.0

[Qemu-devel] [PATCH 1/3] 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>
---
 qapi/qmp-input-visitor.c | 109 +++++++++++++++++++++++++++++++----------------
 1 file changed, 73 insertions(+), 36 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 64dd392..ea9972d 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;
@@ -64,30 +64,34 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
 
     if (QSLIST_EMPTY(&qiv->stack)) {
         /* Starting at root, name is ignored. */
-        return qiv->root;
-    }
-
-    /* We are in a container; find the next element. */
-    tos = QSLIST_FIRST(&qiv->stack);
-    qobj = tos->obj;
-    assert(qobj);
-
-    if (qobject_type(qobj) == QTYPE_QDICT) {
-        assert(name);
-        ret = qdict_get(qobject_to_qdict(qobj), name);
-        if (tos->h && consume && ret) {
-            bool removed = g_hash_table_remove(tos->h, name);
-            assert(removed);
-        }
+        ret = qiv->root;
     } else {
-        assert(qobject_type(qobj) == QTYPE_QLIST);
-        assert(!name);
-        ret = qlist_entry_obj(tos->entry);
-        if (consume) {
-            tos->entry = qlist_next(tos->entry);
+        /* We are in a container; find the next element. */
+        tos = QSLIST_FIRST(&qiv->stack);
+        qobj = tos->obj;
+        assert(qobj);
+
+        if (qobject_type(qobj) == QTYPE_QDICT) {
+            assert(name);
+            ret = qdict_get(qobject_to_qdict(qobj), name);
+            if (tos->h && consume && ret) {
+                bool removed = g_hash_table_remove(tos->h, name);
+                assert(removed);
+            }
+        } else {
+            assert(qobject_type(qobj) == QTYPE_QLIST);
+            assert(!name);
+            ret = qlist_entry_obj(tos->entry);
+            if (consume) {
+                tos->entry = qlist_next(tos->entry);
+            }
         }
     }
 
+    if (!ret) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+    }
+
     return ret;
 }
 
@@ -163,13 +167,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;
@@ -191,10 +198,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;
         }
@@ -232,11 +242,12 @@ 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) {
+    if (obj) {
         *obj = NULL;
-        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+    }
+    if (!qobj) {
         return;
     }
     *obj = g_malloc0(size);
@@ -250,8 +261,12 @@ 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 = qobject_to_qint(qobj);
 
+    if (!qobj) {
+        return;
+    }
     if (!qint) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "integer");
@@ -266,8 +281,12 @@ 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 = qobject_to_qint(qobj);
 
+    if (!qobj) {
+        return;
+    }
     if (!qint) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "integer");
@@ -281,8 +300,12 @@ 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 = qobject_to_qbool(qobj);
 
+    if (!qobj) {
+        return;
+    }
     if (!qbool) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "boolean");
@@ -296,8 +319,12 @@ 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 = qobject_to_qstring(qobj);
 
+    if (!qobj) {
+        return;
+    }
     if (!qstr) {
         *obj = NULL;
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -312,10 +339,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));
@@ -336,7 +366,11 @@ 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);
+
+    if (!qobj) {
+        return;
+    }
 
     qobject_incref(qobj);
     *obj = qobj;
@@ -345,8 +379,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");
@@ -356,7 +393,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 2/3] qapi: clear given pointer

[Marc-André Lureau]

Some getters already set *obj argument to NULL early, let's do this for
all for consistent behaviour in case of errors.

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

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index ea9972d..cb9d196 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -322,11 +322,13 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj,
     QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
     QString *qstr = qobject_to_qstring(qobj);
 
+    if (obj) {
+        *obj = NULL;
+    }
     if (!qobj) {
         return;
     }
     if (!qstr) {
-        *obj = NULL;
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "string");
         return;
@@ -368,6 +370,9 @@ 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, errp);
 
+    if (obj) {
+        *obj = NULL;
+    }
     if (!qobj) {
         return;
     }
-- 
2.10.0

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

[Marc-André Lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.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 1/3] qapi: return a 'missing parameter' error

[Alberto Garcia]

On Wed 21 Sep 2016 12:36:27 PM CEST, 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>

Berto

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Alberto Garcia]

On Wed 21 Sep 2016 12:36:28 PM CEST, Marc-André Lureau wrote:
> Some getters already set *obj argument to NULL early, let's do this for
> all for consistent behaviour in case of errors.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Wed 21 Sep 2016 12:36:29 PM CEST, Marc-André Lureau wrote:

> Missing argument returns a corresponding error message for all types
> now.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 1/3] 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>
> ---
>  qapi/qmp-input-visitor.c | 109 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 73 insertions(+), 36 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 64dd392..ea9972d 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;
> @@ -64,30 +64,34 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>  
>      if (QSLIST_EMPTY(&qiv->stack)) {
>          /* Starting at root, name is ignored. */
> -        return qiv->root;
> -    }

First case: not in a container.

qiv->root cannot be null.

The old code is relatively clear: it returns this non-null value.
Callers rely on it being non-null.

The new code muddies the waters: it handles the impossible null value by
setting an error with a misleading message, then returns null.

Please go back to the old code and simply return qiv->root.  You may
assert it's non-null.

> -
> -    /* We are in a container; find the next element. */
> -    tos = QSLIST_FIRST(&qiv->stack);
> -    qobj = tos->obj;
> -    assert(qobj);
> -
> -    if (qobject_type(qobj) == QTYPE_QDICT) {
> -        assert(name);
> -        ret = qdict_get(qobject_to_qdict(qobj), name);
> -        if (tos->h && consume && ret) {
> -            bool removed = g_hash_table_remove(tos->h, name);
> -            assert(removed);
> -        }
> +        ret = qiv->root;
>      } else {
> -        assert(qobject_type(qobj) == QTYPE_QLIST);
> -        assert(!name);
> -        ret = qlist_entry_obj(tos->entry);
> -        if (consume) {
> -            tos->entry = qlist_next(tos->entry);
> +        /* We are in a container; find the next element. */
> +        tos = QSLIST_FIRST(&qiv->stack);
> +        qobj = tos->obj;
> +        assert(qobj);
> +
> +        if (qobject_type(qobj) == QTYPE_QDICT) {
> +            assert(name);
> +            ret = qdict_get(qobject_to_qdict(qobj), name);
> +            if (tos->h && consume && ret) {
> +                bool removed = g_hash_table_remove(tos->h, name);
> +                assert(removed);
> +            }
> +        } else {
> +            assert(qobject_type(qobj) == QTYPE_QLIST);
> +            assert(!name);
> +            ret = qlist_entry_obj(tos->entry);
> +            if (consume) {
> +                tos->entry = qlist_next(tos->entry);
> +            }
>          }
>      }
>  
> +    if (!ret) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +    }
> +
>      return ret;
>  }

Clearer with whitespace differences ignored:

  @@ -64,9 +64,8 @@

       if (QSLIST_EMPTY(&qiv->stack)) {
           /* Starting at root, name is ignored. */
  -        return qiv->root;
  -    }
  -
  +        ret = qiv->root;
  +    } else {
       /* We are in a container; find the next element. */
       tos = QSLIST_FIRST(&qiv->stack);
       qobj = tos->obj;
       assert(qobj);

       if (qobject_type(qobj) == QTYPE_QDICT) {
           assert(name);
           ret = qdict_get(qobject_to_qdict(qobj), name);
           if (tos->h && consume && ret) {
               bool removed = g_hash_table_remove(tos->h, name);
               assert(removed);
           }
       } else {
           assert(qobject_type(qobj) == QTYPE_QLIST);
           assert(!name);
           ret = qlist_entry_obj(tos->entry);
           if (consume) {
               tos->entry = qlist_next(tos->entry);
           }
       }
  +    }
  +
  +    if (!ret) {
  +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
  +    }

       return ret;
   }

Two more cases:

* In a QTYPE_QDICT container:

  If ret = qdict_get(qobject_to_qdict(qobj, name) is null, parameter
  name is missing, and we want to
  error_setg(errp, QERR_MISSING_PARAMETER, name).  No ternary, because
  name can't be null.

* In a QTYPE_QLIST container:

  ret = qlist_entry_obj(tos->entry) is the list member, a QObject.  It
  must not be null because null is not a valid QObject.  If we want to
  catch this, we should assert, not set an error with a misleading
  message.

Note for the rest of the review: we return null excactly when we set an
error.

>  
> @@ -163,13 +167,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;

Mechanical; the next hunk is the same pattern.

> @@ -191,10 +198,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;
>          }
> @@ -232,11 +242,12 @@ 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) {
> +    if (obj) {
>          *obj = NULL;
> -        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +    }
> +    if (!qobj) {
>          return;
>      }
>      *obj = g_malloc0(size);

Why are you deviating from the mechanical change here?

Note that obj can't be null here, by function contract.  If called via
visit_start_alternate() as it should be, the contract is enforced there.

> @@ -250,8 +261,12 @@ 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 = qobject_to_qint(qobj);
>  
> +    if (!qobj) {
> +        return;
> +    }

I'd call qobject_to_qint() here, not least for consistency with
qmp_input_type_number().  Of course, your code works, and if you feel
strongly about it, we can do it your way here.

>      if (!qint) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "integer");

Mechanical; the next few hunks are the same pattern.

> @@ -266,8 +281,12 @@ 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 = qobject_to_qint(qobj);
>  
> +    if (!qobj) {
> +        return;
> +    }
>      if (!qint) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "integer");
> @@ -281,8 +300,12 @@ 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 = qobject_to_qbool(qobj);
>  
> +    if (!qobj) {
> +        return;
> +    }
>      if (!qbool) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "boolean");
> @@ -296,8 +319,12 @@ 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 = qobject_to_qstring(qobj);
>  
> +    if (!qobj) {
> +        return;
> +    }
>      if (!qstr) {
>          *obj = NULL;
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -312,10 +339,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));
> @@ -336,7 +366,11 @@ 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);
> +
> +    if (!qobj) {
> +        return;
> +    }
>  
>      qobject_incref(qobj);
>      *obj = qobj;

Aha, we got a different bug fix!  The old code fails to fail when the
parameter doesn't exist.  Instead, it sets *obj = NULL, which seems very
likely to crash QEMU.  Let me try... yup:

    { "execute": "object-add",
      "arguments": { "qom-type": "memory-backend-file", "id": "foo" } }

Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type: Assertion `qdict' failed."

Either fix this in a separate patch before this one, or cover it in this
one's commit message.  Your choice.

A separate patch might be usable for qemu-stable.

> @@ -345,8 +379,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.

> @@ -356,7 +393,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;

Thanks for following my suggestion to move the "Parameter FOO is
missing" error into qmp_input_get_object()!  You fixed two crash bugs
that way :)

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Daniel P. Berrange]

On Wed, Sep 21, 2016 at 02:36:28PM +0400, Marc-André Lureau wrote:
> Some getters already set *obj argument to NULL early, let's do this for
> all for consistent behaviour in case of errors.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

If we want consistent behaviour, there's plenty more visit methods
that need updating beyond these two.  eg input_type_int64 will
leave '*obj' untouched on error.

In fact if we want to have '*obj' given a NULL value on error,
then it seems we should instead add code to 'qapi-visit-core.c'
to always initialize '*obj' to NULL, instead of doing it in
qmp-input-visitor.c That way all visitor implementations get
the same behaviour

Alternatively, we could say that '*obj' should never be touched
on error paths, in which case we've a bunchof cleanup todo
in qmp_input_visitor to avoid splattering *obj.

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 1/3] 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>
> > ---
> >  qapi/qmp-input-visitor.c | 109
> >  +++++++++++++++++++++++++++++++----------------
> >  1 file changed, 73 insertions(+), 36 deletions(-)
> >
> > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > index 64dd392..ea9972d 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;
> > @@ -64,30 +64,34 @@ static QObject *qmp_input_get_object(QmpInputVisitor
> > *qiv,
> >  
> >      if (QSLIST_EMPTY(&qiv->stack)) {
> >          /* Starting at root, name is ignored. */
> > -        return qiv->root;
> > -    }
> 
> First case: not in a container.
> 
> qiv->root cannot be null.
> 
> The old code is relatively clear: it returns this non-null value.
> Callers rely on it being non-null.

I didn't realize that, ok

> 
> The new code muddies the waters: it handles the impossible null value by
> setting an error with a misleading message, then returns null.
> 
> Please go back to the old code and simply return qiv->root.  You may
> assert it's non-null.

ok


> Two more cases:
> 
> * In a QTYPE_QDICT container:
> 
>   If ret = qdict_get(qobject_to_qdict(qobj, name) is null, parameter
>   name is missing, and we want to
>   error_setg(errp, QERR_MISSING_PARAMETER, name).  No ternary, because
>   name can't be null.
> 
> * In a QTYPE_QLIST container:
> 
>   ret = qlist_entry_obj(tos->entry) is the list member, a QObject.  It
>   must not be null because null is not a valid QObject.  If we want to
>   catch this, we should assert, not set an error with a misleading
>   message.
> 
> Note for the rest of the review: we return null excactly when we set an
> error.
> 

ok

> >  
> > @@ -163,13 +167,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;
> 
> Mechanical; the next hunk is the same pattern.
> 
> > @@ -191,10 +198,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;
> >          }
> > @@ -232,11 +242,12 @@ 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) {
> > +    if (obj) {
> >          *obj = NULL;
> > -        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> > +    }
> > +    if (!qobj) {
> >          return;
> >      }
> >      *obj = g_malloc0(size);
> 
> Why are you deviating from the mechanical change here?

Because there is already a QERR_MISSING_PARAMETER return.

> 
> Note that obj can't be null here, by function contract.  If called via
> visit_start_alternate() as it should be, the contract is enforced there.

ok

> 
> > @@ -250,8 +261,12 @@ 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 = qobject_to_qint(qobj);
> >  
> > +    if (!qobj) {
> > +        return;
> > +    }
> 
> I'd call qobject_to_qint() here, not least for consistency with
> qmp_input_type_number().  Of course, your code works, and if you feel
> strongly about it, we can do it your way here.

ok

> 
> >      if (!qint) {
> >          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name :
> >          "null",
> >                     "integer");
> 
> Mechanical; the next few hunks are the same pattern.
> 
> > @@ -266,8 +281,12 @@ 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 = qobject_to_qint(qobj);
> >  
> > +    if (!qobj) {
> > +        return;
> > +    }
> >      if (!qint) {
> >          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name :
> >          "null",
> >                     "integer");
> > @@ -281,8 +300,12 @@ 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 = qobject_to_qbool(qobj);
> >  
> > +    if (!qobj) {
> > +        return;
> > +    }
> >      if (!qbool) {
> >          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name :
> >          "null",
> >                     "boolean");
> > @@ -296,8 +319,12 @@ 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 = qobject_to_qstring(qobj);
> >  
> > +    if (!qobj) {
> > +        return;
> > +    }
> >      if (!qstr) {
> >          *obj = NULL;
> >          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name :
> >          "null",
> > @@ -312,10 +339,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));
> > @@ -336,7 +366,11 @@ 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);
> > +
> > +    if (!qobj) {
> > +        return;
> > +    }
> >  
> >      qobject_incref(qobj);
> >      *obj = qobj;
> 
> Aha, we got a different bug fix!  The old code fails to fail when the
> parameter doesn't exist.  Instead, it sets *obj = NULL, which seems very
> likely to crash QEMU.  Let me try... yup:
> 
>     { "execute": "object-add",
>       "arguments": { "qom-type": "memory-backend-file", "id": "foo" } }
> 
> Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type:
> Assertion `qdict' failed."
> 
> Either fix this in a separate patch before this one, or cover it in this
> one's commit message.  Your choice.

ok, I'll make a seperate patch

> 
> A separate patch might be usable for qemu-stable.
> 
> > @@ -345,8 +379,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.

let's include it in the same patch

> 
> > @@ -356,7 +393,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;
> 
> Thanks for following my suggestion to move the "Parameter FOO is
> missing" error into qmp_input_get_object()!  You fixed two crash bugs
> that way :)
> 

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

[Markus Armbruster]

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

> Missing argument returns a corresponding error message for all types
> now.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.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"}

Did this regress in PATCH 1?

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Marc-André Lureau]

Hi

----- Original Message -----
> On Wed, Sep 21, 2016 at 02:36:28PM +0400, Marc-André Lureau wrote:
> > Some getters already set *obj argument to NULL early, let's do this for
> > all for consistent behaviour in case of errors.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> If we want consistent behaviour, there's plenty more visit methods
> that need updating beyond these two.  eg input_type_int64 will
> leave '*obj' untouched on error.
> 
> In fact if we want to have '*obj' given a NULL value on error,
> then it seems we should instead add code to 'qapi-visit-core.c'
> to always initialize '*obj' to NULL, instead of doing it in
> qmp-input-visitor.c That way all visitor implementations get
> the same behaviour

I think that's not easily doable, as an input visitor will want to set *obj (to NULL or something), but the output visitor may need *obj != NULL as an input.

It'snot really elegant that there is visitor-input/output specific code already in the visit-core, I would rather have that code in the respective visitors.

> 
> Alternatively, we could say that '*obj' should never be touched
> on error paths, in which case we've a bunchof cleanup todo
> in qmp_input_visitor to avoid splattering *obj.
> 
> 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 2/3] qapi: clear given pointer

[Markus Armbruster]

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

> Some getters already set *obj argument to NULL early, let's do this for
> all for consistent behaviour in case of errors.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/qmp-input-visitor.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index ea9972d..cb9d196 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -322,11 +322,13 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj,
>      QObject *qobj = qmp_input_get_object(qiv, name, true, errp);
>      QString *qstr = qobject_to_qstring(qobj);
>  
> +    if (obj) {
> +        *obj = NULL;
> +    }
>      if (!qobj) {
>          return;
>      }
>      if (!qstr) {
> -        *obj = NULL;
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "string");
>          return;

This undoes damage done in PATCH 1.

> @@ -368,6 +370,9 @@ 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, errp);
>  
> +    if (obj) {
> +        *obj = NULL;
> +    }
>      if (!qobj) {
>          return;
>      }

Likewise.

Similar damage done to qmp_input_start_list() and possibly others.
Please squash into PATCH 1 and double-check your new error returns affect
*obj like the existing ones.

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

[Marc-André Lureau]

Hi

----- Original Message -----
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Missing argument returns a corresponding error message for all types
> > now.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.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"}
> 
> Did this regress in PATCH 1?

If you look at old monitor dispatch code, it does not return QERR_MISSING_PARAMETER for structure members, but only top level arguments. I don't think it's worth to keep that old error behaviour. I will update the commit message. 

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Daniel P. Berrange]

On Wed, Sep 21, 2016 at 11:17:45AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Wed, Sep 21, 2016 at 02:36:28PM +0400, Marc-André Lureau wrote:
> > > Some getters already set *obj argument to NULL early, let's do this for
> > > all for consistent behaviour in case of errors.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > If we want consistent behaviour, there's plenty more visit methods
> > that need updating beyond these two.  eg input_type_int64 will
> > leave '*obj' untouched on error.
> > 
> > In fact if we want to have '*obj' given a NULL value on error,
> > then it seems we should instead add code to 'qapi-visit-core.c'
> > to always initialize '*obj' to NULL, instead of doing it in
> > qmp-input-visitor.c That way all visitor implementations get
> > the same behaviour
> 
> I think that's not easily doable, as an input visitor will want to set *obj (to NULL or something), but the output visitor may need *obj != NULL as an input.

Oh good point.

> It'snot really elegant that there is visitor-input/output specific code already in the visit-core, I would rather have that code in the respective visitors.


Also, my series of visitor patches will delete opts-visitor and
string-input-visitor, so ultimately qmp-input-visitor will be
the only one left doing input work.


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 1/3] qapi: return a 'missing parameter' error

[Marc-André Lureau]

Hi

----- Original Message -----
> Aha, we got a different bug fix!  The old code fails to fail when the
> parameter doesn't exist.  Instead, it sets *obj = NULL, which seems very
> likely to crash QEMU.  Let me try... yup:
> 
>     { "execute": "object-add",
>       "arguments": { "qom-type": "memory-backend-file", "id": "foo" } }
> 
> Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type:
> Assertion `qdict' failed."
> 
> Either fix this in a separate patch before this one, or cover it in this
> one's commit message.  Your choice.
> 
> A separate patch might be usable for qemu-stable.

It looks to me that this is a different bug. 

visit_type_q_obj_object_add_arg_members() doesn't call visit_type_any() if "props" is missing (it's optionnal).

And arg is zero'ed in qmp-marshal, and the assert() was added in ad739706bbadee49. I am trying to fix that regression.

> 
> > @@ -345,8 +379,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.
> 
> > @@ -356,7 +393,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;
> 
> Thanks for following my suggestion to move the "Parameter FOO is
> missing" error into qmp_input_get_object()!  You fixed two crash bugs
> that way :)
> 

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

[Markus Armbruster]

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

> Hi
>
> ----- Original Message -----
>> Aha, we got a different bug fix!  The old code fails to fail when the
>> parameter doesn't exist.  Instead, it sets *obj = NULL, which seems very
>> likely to crash QEMU.  Let me try... yup:
>> 
>>     { "execute": "object-add",
>>       "arguments": { "qom-type": "memory-backend-file", "id": "foo" } }
>> 
>> Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type:
>> Assertion `qdict' failed."
>> 
>> Either fix this in a separate patch before this one, or cover it in this
>> one's commit message.  Your choice.
>> 
>> A separate patch might be usable for qemu-stable.
>
> It looks to me that this is a different bug. 
>
> visit_type_q_obj_object_add_arg_members() doesn't call visit_type_any() if "props" is missing (it's optionnal).
>
> And arg is zero'ed in qmp-marshal, and the assert() was added in ad739706bbadee49. I am trying to fix that regression.

Okay, that's *also* a bug.

For the bug I spotted, try

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

Trips assert(!err != !*obj) in its caller visit_type_any().

[...]