diff mbox

[v6,15/18] monitor: use qmp_dispatch()

Message ID 81179735.279206.1474408839927.JavaMail.zimbra@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc-André Lureau Sept. 20, 2016, 10 p.m. UTC 
----- Original Message -----
> Hi Alberto
> 
> ----- Original Message -----
> > On Mon, Sep 12, 2016 at 01:19:10PM +0400, Marc-André Lureau wrote:
> > > Replace the old manual dispatch and validation code by the generic one
> > > provided by qapi common code.
> > > 
> > > Note that it is now possible to call the following commands that used to
> > > be disabled by compile-time conditionals:
> > > - dump-skeys
> > > - query-spice
> > > - rtc-reset-reinjection
> > > - query-gic-capabilities
> > > 
> > > Their fallback functions return an appropriate "feature disabled" error.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > This patch breaks iotest 085 because the "missing parameter" error is
> > now different:
> > 
> > -{"error": {"class": "GenericError", "desc": "Parameter 'snapshot-file' is
> > missing"}}
> > +{"error": {"class": "GenericError", "desc": "Invalid parameter type for
> > 'snapshot-file', expected: string"}}
> > 
> > I was thinking to update the expected output of the iotest, but I
> > guess it's better to return a more meaningful error message?
> 
> The change is relatively easy (see attached patch), but there are other tests
> that expected the current error, see commit fe509ee237307843. I guess we
> should decided with one, and I think missing parameter is more appropriate.
> 

diff attached

Comments

Markus Armbruster Sept. 21, 2016, 6:15 a.m. UTC | #1 
Marc-André Lureau <mlureau@redhat.com> writes:

> ----- Original Message -----
>> Hi Alberto
>> 
>> ----- Original Message -----
>> > On Mon, Sep 12, 2016 at 01:19:10PM +0400, Marc-André Lureau wrote:
>> > > Replace the old manual dispatch and validation code by the generic one
>> > > provided by qapi common code.
>> > > 
>> > > Note that it is now possible to call the following commands that used to
>> > > be disabled by compile-time conditionals:
>> > > - dump-skeys
>> > > - query-spice
>> > > - rtc-reset-reinjection
>> > > - query-gic-capabilities
>> > > 
>> > > Their fallback functions return an appropriate "feature disabled" error.
>> > > 
>> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > 
>> > This patch breaks iotest 085 because the "missing parameter" error is
>> > now different:
>> > 
>> > -{"error": {"class": "GenericError", "desc": "Parameter 'snapshot-file' is
>> > missing"}}
>> > +{"error": {"class": "GenericError", "desc": "Invalid parameter type for
>> > 'snapshot-file', expected: string"}}
>> > 
>> > I was thinking to update the expected output of the iotest, but I
>> > guess it's better to return a more meaningful error message?
>> 
>> The change is relatively easy (see attached patch), but there are other tests
>> that expected the current error, see commit fe509ee237307843. I guess we
>> should decided with one, and I think missing parameter is more appropriate.

Agree.

> diff attached
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 64dd392..22b132b 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -169,7 +169,11 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
>      if (obj) {
>          *obj = NULL;
>      }
> -    if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
> +    if (qobject_type(qobj) != QTYPE_QDICT) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "QDict");
>          return;
> @@ -194,7 +198,11 @@ static void qmp_input_start_list(Visitor *v, const char *name,
>      QObject *qobj = qmp_input_get_object(qiv, name, true);
>      const QListEntry *entry;
>  
> -    if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
> +    if (qobject_type(qobj) != QTYPE_QLIST) {
>          if (list) {
>              *list = NULL;
>          }
> @@ -234,8 +242,10 @@ static void qmp_input_start_alternate(Visitor *v, const char *name,
>      QmpInputVisitor *qiv = to_qiv(v);
>      QObject *qobj = qmp_input_get_object(qiv, name, false);
>  
> -    if (!qobj) {
> +    if (obj) {
>          *obj = NULL;
> +    }
> +    if (!qobj) {
>          error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
>          return;
>      }
> @@ -250,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);
> +    QInt *qint = qobject_to_qint(qobj);
>  
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
>      if (!qint) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "integer");
> @@ -266,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);
> +    QInt *qint = qobject_to_qint(qobj);
>  
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
>      if (!qint) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "integer");
> @@ -281,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);
> +    QBool *qbool = qobject_to_qbool(qobj);
>  
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
>      if (!qbool) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "boolean");
> @@ -296,8 +321,16 @@ 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);
> +    QString *qstr = qobject_to_qstring(qobj);
>  
> +    if (obj) {
> +        *obj = NULL;
> +    }
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
>      if (!qstr) {
>          *obj = NULL;
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -316,6 +349,10 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj,
>      QInt *qint;
>      QFloat *qfloat;
>  
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
>      qint = qobject_to_qint(qobj);
>      if (qint) {
>          *obj = qint_get_int(qobject_to_qint(qobj));
> @@ -338,6 +375,14 @@ 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 (obj) {
> +        *obj = NULL;
> +    }
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
> +
>      qobject_incref(qobj);
>      *obj = qobj;
>  }
> @@ -347,6 +392,10 @@ 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);
>  
> +    if (!qobj) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> +        return;
> +    }
>      if (qobject_type(qobj) != QTYPE_QNULL) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "null");

Changing qmp_input_get_object() to take an Error * parameter and set the
"Parameter FOO is missing" error could require less code.  Suggest to
give it a try and see how you like it.

Need a proper patch to merge this regression fix.
diff mbox

Patch

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 64dd392..22b132b 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -169,7 +169,11 @@  static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
     if (obj) {
         *obj = NULL;
     }
-    if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
+    if (qobject_type(qobj) != QTYPE_QDICT) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "QDict");
         return;
@@ -194,7 +198,11 @@  static void qmp_input_start_list(Visitor *v, const char *name,
     QObject *qobj = qmp_input_get_object(qiv, name, true);
     const QListEntry *entry;
 
-    if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
+    if (qobject_type(qobj) != QTYPE_QLIST) {
         if (list) {
             *list = NULL;
         }
@@ -234,8 +242,10 @@  static void qmp_input_start_alternate(Visitor *v, const char *name,
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, false);
 
-    if (!qobj) {
+    if (obj) {
         *obj = NULL;
+    }
+    if (!qobj) {
         error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
         return;
     }
@@ -250,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);
+    QInt *qint = qobject_to_qint(qobj);
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
     if (!qint) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "integer");
@@ -266,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);
+    QInt *qint = qobject_to_qint(qobj);
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
     if (!qint) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "integer");
@@ -281,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);
+    QBool *qbool = qobject_to_qbool(qobj);
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
     if (!qbool) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "boolean");
@@ -296,8 +321,16 @@  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);
+    QString *qstr = qobject_to_qstring(qobj);
 
+    if (obj) {
+        *obj = NULL;
+    }
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
     if (!qstr) {
         *obj = NULL;
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -316,6 +349,10 @@  static void qmp_input_type_number(Visitor *v, const char *name, double *obj,
     QInt *qint;
     QFloat *qfloat;
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
     qint = qobject_to_qint(qobj);
     if (qint) {
         *obj = qint_get_int(qobject_to_qint(qobj));
@@ -338,6 +375,14 @@  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 (obj) {
+        *obj = NULL;
+    }
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
+
     qobject_incref(qobj);
     *obj = qobj;
 }
@@ -347,6 +392,10 @@  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);
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
     if (qobject_type(qobj) != QTYPE_QNULL) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "null");