[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error |
Date: |
Wed, 05 Oct 2016 15:40:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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_MISSING_PARAMETER where
> appropriate.
>
> Fix expected error message in iotests.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
> qapi/qmp-input-visitor.c | 69
> ++++++++++++++++++++++++++++++++++------------
> tests/qemu-iotests/087.out | 2 +-
> 2 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index fe097c9..9b9b350 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);
Note for later: qmp_input_get_object() either returns non-null and
leaves @errp alone, or it returns null and sets an error.
> @@ -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);
>
> 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,11 +369,15 @@ 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;
> + }
>
> if (!qobj) {
> error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
> - *obj = NULL;
> return;
> }
The second conditional is dead. Mismerge?
The obvious fix is to drop it. Can do on commit.
>
> @@ -353,7 +388,7 @@ 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) {
> error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
return;
When qmp_input_get_object() returns null, it also sets an error. The
conditional then sets an error again, which is wrong and can fail the
assertion in error_setv().
The obvious fix is to drop the error_setg(). Can do on commit.
}
if (qobject_type(qobj) != QTYPE_QNULL) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"null");
}
}
> @@ -369,7 +404,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;
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> index bef6862..55ba6a7 100644
> --- a/tests/qemu-iotests/087.out
> +++ b/tests/qemu-iotests/087.out
> @@ -56,7 +56,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"}
- Re: [Qemu-devel] [PATCH v3 3/3] qapi: return a 'missing parameter' error,
Markus Armbruster <=