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

[Markus Armbruster]

Marc-André Lureau <address@hidden> 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 <address@hidden>
> ---
>  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 :)