* [Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error
@ 2016-09-21 20:10 Marc-André Lureau
2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value Marc-André Lureau
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Marc-André Lureau @ 2016-09-21 20:10 UTC (permalink / raw)
To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau
Hi,
'monitor: use qmp_dispatch()' patch broke some iotests expecting a
'missing parameter' error. This series fixes qapi visitors to return
this error for all types.
v2:
- add some assert() for expected values in qmp-input-visitor, making
it more explicit when qmp_input_get_object() may return NULL
- squash the *obj = NULL changes with the missing arg error patch
- move qobject_to_*(qobj) calls after checking qobj != NULL
- update commit messages
Marc-André Lureau (4):
qapi: add assert about root value
qapi: assert list entry has a value
qapi: return a 'missing parameter' error
iotests: fix expected error message
qapi/qmp-input-visitor.c | 76 +++++++++++++++++++++++++++++++++++-----------
tests/qemu-iotests/087.out | 2 +-
2 files changed, 60 insertions(+), 18 deletions(-)
--
2.10.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value 2016-09-21 20:10 [Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error Marc-André Lureau @ 2016-09-21 20:10 ` Marc-André Lureau 2016-09-21 21:05 ` Eric Blake 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value Marc-André Lureau ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Marc-André Lureau @ 2016-09-21 20:10 UTC (permalink / raw) To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau qiv->root should not be null, make that clearer with some assert. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- qapi/qmp-input-visitor.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 64dd392..be52aaf 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -64,6 +64,7 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, if (QSLIST_EMPTY(&qiv->stack)) { /* Starting at root, name is ignored. */ + assert(qiv->root); return qiv->root; } @@ -384,6 +385,7 @@ Visitor *qmp_input_visitor_new(QObject *obj, bool strict) { QmpInputVisitor *v; + assert(obj); v = g_malloc0(sizeof(*v)); v->visitor.type = VISITOR_INPUT; -- 2.10.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value Marc-André Lureau @ 2016-09-21 21:05 ` Eric Blake 0 siblings, 0 replies; 13+ messages in thread From: Eric Blake @ 2016-09-21 21:05 UTC (permalink / raw) To: Marc-André Lureau, qemu-devel; +Cc: berto, armbru [-- Attachment #1: Type: text/plain, Size: 432 bytes --] On 09/21/2016 03:10 PM, Marc-André Lureau wrote: > qiv->root should not be null, make that clearer with some assert. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/qmp-input-visitor.c | 2 ++ > 1 file changed, 2 insertions(+) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value 2016-09-21 20:10 [Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error Marc-André Lureau 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value Marc-André Lureau @ 2016-09-21 20:10 ` Marc-André Lureau 2016-09-21 21:06 ` Eric Blake 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error Marc-André Lureau 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message Marc-André Lureau 3 siblings, 1 reply; 13+ messages in thread From: Marc-André Lureau @ 2016-09-21 20:10 UTC (permalink / raw) To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau This helps to figure out the expectations. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- qapi/qmp-input-visitor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index be52aaf..c1019d6 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -84,6 +84,7 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, assert(qobject_type(qobj) == QTYPE_QLIST); assert(!name); ret = qlist_entry_obj(tos->entry); + assert(ret); if (consume) { tos->entry = qlist_next(tos->entry); } -- 2.10.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value Marc-André Lureau @ 2016-09-21 21:06 ` Eric Blake 0 siblings, 0 replies; 13+ messages in thread From: Eric Blake @ 2016-09-21 21:06 UTC (permalink / raw) To: Marc-André Lureau, qemu-devel; +Cc: berto, armbru [-- Attachment #1: Type: text/plain, Size: 910 bytes --] On 09/21/2016 03:10 PM, Marc-André Lureau wrote: > This helps to figure out the expectations. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/qmp-input-visitor.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index be52aaf..c1019d6 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -84,6 +84,7 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > assert(qobject_type(qobj) == QTYPE_QLIST); > assert(!name); > ret = qlist_entry_obj(tos->entry); > + assert(ret); > if (consume) { > tos->entry = qlist_next(tos->entry); > } > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error 2016-09-21 20:10 [Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error Marc-André Lureau 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value Marc-André Lureau 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value Marc-André Lureau @ 2016-09-21 20:10 ` Marc-André Lureau 2016-09-21 21:35 ` Eric Blake ` (2 more replies) 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message Marc-André Lureau 3 siblings, 3 replies; 13+ messages in thread From: Marc-André Lureau @ 2016-09-21 20:10 UTC (permalink / raw) To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau 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.lureau@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> --- qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 17 deletions(-) diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index c1019d6..6f85664 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); @@ -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); + *obj = NULL; 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,7 +369,12 @@ 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; + } qobject_incref(qobj); *obj = qobj; @@ -347,8 +383,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"); @@ -358,7 +397,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; -- 2.10.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error Marc-André Lureau @ 2016-09-21 21:35 ` Eric Blake 2016-09-22 8:06 ` Daniel P. Berrange 2016-09-22 12:46 ` Markus Armbruster 2 siblings, 0 replies; 13+ messages in thread From: Eric Blake @ 2016-09-21 21:35 UTC (permalink / raw) To: Marc-André Lureau, qemu-devel; +Cc: berto, armbru [-- Attachment #1: Type: text/plain, Size: 721 bytes --] On 09/21/2016 03:10 PM, Marc-André Lureau wrote: > 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.lureau@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 56 insertions(+), 17 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error Marc-André Lureau 2016-09-21 21:35 ` Eric Blake @ 2016-09-22 8:06 ` Daniel P. Berrange 2016-09-22 12:46 ` Markus Armbruster 2 siblings, 0 replies; 13+ messages in thread From: Daniel P. Berrange @ 2016-09-22 8:06 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, berto, armbru On Thu, Sep 22, 2016 at 12:10:17AM +0400, Marc-André Lureau wrote: > 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. Surely this is meant to say 'return QERR_MISSING_PARAMETER', as QERR_INVALID_PARAMETER_TYPE is what its already returning. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 56 insertions(+), 17 deletions(-) There's a pretty detailed unit test for this visitor, which should be further extended to cover this new behaviour. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error Marc-André Lureau 2016-09-21 21:35 ` Eric Blake 2016-09-22 8:06 ` Daniel P. Berrange @ 2016-09-22 12:46 ` Markus Armbruster 2016-09-22 12:58 ` Marc-André Lureau 2 siblings, 1 reply; 13+ messages in thread From: Markus Armbruster @ 2016-09-22 12:46 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, berto Marc-André Lureau <marcandre.lureau@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.lureau@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > qapi/qmp-input-visitor.c | 73 +++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 56 insertions(+), 17 deletions(-) > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index c1019d6..6f85664 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); > @@ -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); > > + *obj = NULL; > if (!qobj) { > - *obj = NULL; > - error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > return; > } > *obj = g_malloc0(size); The patch does more than one thing: in addition to fixing the 'missing parameter' regression, it also messes with *obj = NULL in places. These changes may well make sense, but they should be a separate patch, to ease review. > @@ -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,7 +369,12 @@ 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; > + } > > qobject_incref(qobj); > *obj = qobj; The patch does a third thing: it fixes a crash bug. The old code fails to fail when the parameter doesn't exist. Instead, it sets *obj = NULL, violating its contract. Reproducer: { "execute": "qom-set", "arguments": { "path": "/machine", "property": "rtc-time" } } Separate patch, please, cc: qemu-stable. > @@ -347,8 +383,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. > @@ -358,7 +397,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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error 2016-09-22 12:46 ` Markus Armbruster @ 2016-09-22 12:58 ` Marc-André Lureau 0 siblings, 0 replies; 13+ messages in thread From: Marc-André Lureau @ 2016-09-22 12:58 UTC (permalink / raw) To: Markus Armbruster; +Cc: Marc-André Lureau, qemu-devel, berto Hi ----- Original Message ----- > Marc-André Lureau <marcandre.lureau@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.lureau@redhat.com> > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > --- > > qapi/qmp-input-visitor.c | 73 > > +++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 56 insertions(+), 17 deletions(-) > > > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > > index c1019d6..6f85664 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); > > @@ -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); > > > > + *obj = NULL; > > if (!qobj) { > > - *obj = NULL; > > - error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > > return; > > } > > *obj = g_malloc0(size); > > The patch does more than one thing: in addition to fixing the 'missing > parameter' regression, it also messes with *obj = NULL in places. These > changes may well make sense, but they should be a separate patch, to > ease review. Looking more at it, I think we can leave it only in the error case. > > > @@ -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,7 +369,12 @@ 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; > > + } > > > > qobject_incref(qobj); > > *obj = qobj; > > The patch does a third thing: it fixes a crash bug. The old code fails > to fail when the parameter doesn't exist. Instead, it sets *obj = NULL, > violating its contract. Reproducer: > > { "execute": "qom-set", > "arguments": { "path": "/machine", "property": "rtc-time" } } > > Separate patch, please, cc: qemu-stable. > We can make it a seperate patch, but we still need to return an error, that's what this patch does. A seperate patch will look like: @@ -338,6 +338,12 @@ 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 (!qobj) { + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); + *obj = NULL; + return; + } I think we may as well just take this patch, what do you think? > > @@ -347,8 +383,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. > > > @@ -358,7 +397,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; > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message 2016-09-21 20:10 [Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error Marc-André Lureau ` (2 preceding siblings ...) 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error Marc-André Lureau @ 2016-09-21 20:10 ` Marc-André Lureau 2016-09-21 21:33 ` Eric Blake 3 siblings, 1 reply; 13+ messages in thread From: Marc-André Lureau @ 2016-09-21 20:10 UTC (permalink / raw) To: qemu-devel; +Cc: berto, eblake, armbru, Marc-André Lureau Missing argument returns a corresponding error message for all types now. The "old" qmp dispatch code didn't return QERR_MISSING_PARAMETER for argument structure members (it returned QERR_INVALID_PARAMETER). Only for top-level argument it did return QERR_MISSING_PARAMETER. It is preferable to have a consistent error for missing fields in inner structs as well. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> --- tests/qemu-iotests/087.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out index a95c4b0..b213db2 100644 --- a/tests/qemu-iotests/087.out +++ b/tests/qemu-iotests/087.out @@ -60,7 +60,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"} -- 2.10.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message Marc-André Lureau @ 2016-09-21 21:33 ` Eric Blake 2016-09-21 21:44 ` Marc-André Lureau 0 siblings, 1 reply; 13+ messages in thread From: Eric Blake @ 2016-09-21 21:33 UTC (permalink / raw) To: Marc-André Lureau, qemu-devel; +Cc: berto, armbru [-- Attachment #1: Type: text/plain, Size: 1045 bytes --] On 09/21/2016 03:10 PM, Marc-André Lureau wrote: > Missing argument returns a corresponding error message for all types > now. > > The "old" qmp dispatch code didn't return QERR_MISSING_PARAMETER for > argument structure members (it returned QERR_INVALID_PARAMETER). Only > for top-level argument it did return QERR_MISSING_PARAMETER. It is > preferable to have a consistent error for missing fields in inner > structs as well. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > tests/qemu-iotests/087.out | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Yes, this looks better. If the message is changed earlier in this series, then this should be squashed into that patch (3/4?); if the message changed as the result of an earlier commit, the commit message should call out the id at which the tree was temporarily broken. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message 2016-09-21 21:33 ` Eric Blake @ 2016-09-21 21:44 ` Marc-André Lureau 0 siblings, 0 replies; 13+ messages in thread From: Marc-André Lureau @ 2016-09-21 21:44 UTC (permalink / raw) To: Eric Blake; +Cc: Marc-André Lureau, qemu-devel, berto, armbru ----- Original Message ----- > On 09/21/2016 03:10 PM, Marc-André Lureau wrote: > > Missing argument returns a corresponding error message for all types > > now. > > > > The "old" qmp dispatch code didn't return QERR_MISSING_PARAMETER for > > argument structure members (it returned QERR_INVALID_PARAMETER). Only > > for top-level argument it did return QERR_MISSING_PARAMETER. It is > > preferable to have a consistent error for missing fields in inner > > structs as well. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > --- > > tests/qemu-iotests/087.out | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Yes, this looks better. > > If the message is changed earlier in this series, then this should be > squashed into that patch (3/4?); if the message changed as the result of > an earlier commit, the commit message should call out the id at which > the tree was temporarily broken. I checked, it's the previous patch that introduce the change (I wasn't sure given that in alternate case qapi already returned MISSING_PARAMETER), it can be squashed with it. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-09-22 12:59 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-21 20:10 [Qemu-devel] [PATCH v2 0/4] qapi: return 'missing parameter' error Marc-André Lureau 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 1/4] qapi: add assert about root value Marc-André Lureau 2016-09-21 21:05 ` Eric Blake 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 2/4] qapi: assert list entry has a value Marc-André Lureau 2016-09-21 21:06 ` Eric Blake 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 3/4] qapi: return a 'missing parameter' error Marc-André Lureau 2016-09-21 21:35 ` Eric Blake 2016-09-22 8:06 ` Daniel P. Berrange 2016-09-22 12:46 ` Markus Armbruster 2016-09-22 12:58 ` Marc-André Lureau 2016-09-21 20:10 ` [Qemu-devel] [PATCH v2 4/4] iotests: fix expected error message Marc-André Lureau 2016-09-21 21:33 ` Eric Blake 2016-09-21 21:44 ` Marc-André Lureau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).