[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] qapi: return a 'missing parameter' error
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] qapi: return a 'missing parameter' error |
Date: |
Wed, 21 Sep 2016 11:05:09 -0400 (EDT) |
Hi
----- Original Message -----
> 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>
> > ---
> > 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 :)
>
[Qemu-devel] [PATCH 3/3] iotests: fix expected error message, Marc-André Lureau, 2016/09/21