Marc-André Lureau <marcandre.lur...@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.lur...@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 :)
