Re: [Qemu-devel] [PATCH v2 3/4] 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>
> Reviewed-by: Alberto Garcia <address@hidden>
> ---
>  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;