Hi ----- Original Message ----- > 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.
I didn't realize that, ok > > 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. ok > 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. > ok > > > > @@ -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? Because there is already a QERR_MISSING_PARAMETER return. > > 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. ok > > > @@ -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. ok > > > 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. ok, I'll make a seperate patch > > 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. let's include it in the same patch > > > @@ -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 :) >