| Message ID | 1461801715-24307-6-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Rather than having two separate ways to create a QMP input > visitor, where the safer approach has the more verbose name, > it is better to consolidate things into a single function > where the caller must explicitly choose whether to be strict > or to ignore excess input. Use strict mode in more places of > internal code (such as when cloning a QAPI struct in > util/socket.c, where the QObject better not have excess > input since it was just generated by qmp-output), while > documenting in user-facing code a question of whether we > should change our policy about ignoring excess input. Which external interface is this? > > In the case of qmp_object_add(), we intentionally switch to a > strict visitor; this matches the fact that the code for > user_creatable_add_type() is shared by both qmp_object_add() > (QMP input visitor) and by user_creatable_add_opts() (QemuOpts > visitor); the latter is always strict, so our usage in the > former should also be strict, so that both visits will > equally diagnose any excess input in a nested dict. But in > practice, we don't really have any -object where the > properties are a nested dict, and excess input at the top > level is already caught earlier by object_property_set() on > an unknown key, whether from QemuOpts: > > $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio -object secret,id=sec0,data=letmein,format=raw,foo=bar > qemu-system-x86_64: Property '.foo' not found Let's update the error message now that the error message regression is fixed. Can do on commit. > > or from QMP: > > $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio > {"QMP": {"version": {"qemu": {"micro": 93, "minor": 5, "major": 2}, "package": ""}, "capabilities": []}} > {"execute":"qmp_capabilities"} > {"return": {}} > {"execute":"object-add","arguments":{"qom-type":"secret","id":"sec0","props":{"format":"raw","data":"letmein","a":1}}} > {"error": {"class": "GenericError", "desc": "Property '.a' not found"}} > > Signed-off-by: Eric Blake <eblake@redhat.com> Should we split this into a patch to change the interface, and one or more separate patches to switch to the strict visitor? Could result in clearer and more complete commit messages. > > --- > v15: new patch "v15: new patch" *groan* > --- > scripts/qapi-commands.py | 2 +- > include/qapi/qmp-input-visitor.h | 9 +++++++-- > qapi/qmp-input-visitor.c | 13 ++----------- > qmp.c | 2 +- > qom/qom-qobject.c | 3 ++- > replay/replay-input.c | 2 +- > tests/test-qmp-commands.c | 2 +- > tests/test-qmp-input-strict.c | 2 +- > tests/test-qmp-input-visitor.c | 2 +- > tests/test-visitor-serialization.c | 2 +- > util/qemu-sockets.c | 2 +- > docs/qapi-code-gen.txt | 2 +- > 12 files changed, 20 insertions(+), 23 deletions(-) > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index b570069..6261e44 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -115,7 +115,7 @@ def gen_marshal(name, arg_type, ret_type): > > if arg_type and arg_type.members: > ret += mcgen(''' > - QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); > + QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); > QapiDeallocVisitor *qdv; > Visitor *v; > %(c_name)s arg = {0}; Stay strict. > diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h > index 3ed499c..b0624d8 100644 > --- a/include/qapi/qmp-input-visitor.h > +++ b/include/qapi/qmp-input-visitor.h > @@ -19,8 +19,13 @@ > > typedef struct QmpInputVisitor QmpInputVisitor; > > -QmpInputVisitor *qmp_input_visitor_new(QObject *obj); > -QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj); > +/* > + * Return a new input visitor that converts QMP to QAPI. > + * > + * Set @strict to reject a parse that doesn't consume all keys of a > + * dictionary; otherwise excess input is ignored. > + */ > +QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool strict); > > void qmp_input_visitor_cleanup(QmpInputVisitor *v); > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 8550bc7..c3c3271 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -356,7 +356,7 @@ void qmp_input_visitor_cleanup(QmpInputVisitor *v) > g_free(v); > } > > -QmpInputVisitor *qmp_input_visitor_new(QObject *obj) > +QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool strict) > { > QmpInputVisitor *v; > > @@ -376,19 +376,10 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) > v->visitor.type_number = qmp_input_type_number; > v->visitor.type_any = qmp_input_type_any; > v->visitor.optional = qmp_input_optional; > + v->strict = strict; > > qmp_input_push(v, obj, NULL); > qobject_incref(obj); > > return v; > } > - > -QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj) > -{ > - QmpInputVisitor *v; > - > - v = qmp_input_visitor_new(obj); > - v->strict = true; > - > - return v; > -} > diff --git a/qmp.c b/qmp.c > index 9d0953b..e784a67 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -663,7 +663,7 @@ void qmp_object_add(const char *type, const char *id, > } > } > > - qiv = qmp_input_visitor_new(props); > + qiv = qmp_input_visitor_new(props, true); > obj = user_creatable_add_type(type, id, pdict, > qmp_input_get_visitor(qiv), errp); > qmp_input_visitor_cleanup(qiv); Switch to strict. The commit message explains why this is a good idea. > diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c > index e6b17c1..b66088d 100644 > --- a/qom/qom-qobject.c > +++ b/qom/qom-qobject.c > @@ -22,7 +22,8 @@ void object_property_set_qobject(Object *obj, QObject *value, > const char *name, Error **errp) > { > QmpInputVisitor *qiv; > - qiv = qmp_input_visitor_new(value); > + /* TODO: Should we reject, rather than ignore, excess input? */ > + qiv = qmp_input_visitor_new(value, false); > object_property_set(obj, qmp_input_get_visitor(qiv), name, errp); > > qmp_input_visitor_cleanup(qiv); Stay lenient, but document this should perhaps switch to strict. The commit message hints at this one. > diff --git a/replay/replay-input.c b/replay/replay-input.c > index 06babe0..03e99d5 100644 > --- a/replay/replay-input.c > +++ b/replay/replay-input.c > @@ -37,7 +37,7 @@ static InputEvent *qapi_clone_InputEvent(InputEvent *src) > return NULL; > } > > - qiv = qmp_input_visitor_new(obj); > + qiv = qmp_input_visitor_new(obj, true); > iv = qmp_input_get_visitor(qiv); > visit_type_InputEvent(iv, NULL, &dst, &error_abort); > qmp_input_visitor_cleanup(qiv); Switch to strict. Not mentioned in commit message. Why is it a good idea? > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c > index 14a9ebb..597fb44 100644 > --- a/tests/test-qmp-commands.c > +++ b/tests/test-qmp-commands.c > @@ -222,7 +222,7 @@ static void test_dealloc_partial(void) > ud2_dict = qdict_new(); > qdict_put_obj(ud2_dict, "string0", QOBJECT(qstring_from_str(text))); > > - qiv = qmp_input_visitor_new(QOBJECT(ud2_dict)); > + qiv = qmp_input_visitor_new(QOBJECT(ud2_dict), true); > visit_type_UserDefTwo(qmp_input_get_visitor(qiv), NULL, &ud2, &err); > qmp_input_visitor_cleanup(qiv); > QDECREF(ud2_dict); Switch to strict. Not mentioned in commit message. Why is it a good idea? > diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c > index d5f80ec..2b053a2 100644 > --- a/tests/test-qmp-input-strict.c > +++ b/tests/test-qmp-input-strict.c > @@ -55,7 +55,7 @@ static Visitor *validate_test_init_internal(TestInputVisitorData *data, > data->obj = qobject_from_jsonv(json_string, ap); > g_assert(data->obj); > > - data->qiv = qmp_input_visitor_new_strict(data->obj); > + data->qiv = qmp_input_visitor_new(data->obj, true); > g_assert(data->qiv); > > v = qmp_input_get_visitor(data->qiv); Stay strict (because we're testing it). > diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c > index 80527eb..c039806 100644 > --- a/tests/test-qmp-input-visitor.c > +++ b/tests/test-qmp-input-visitor.c > @@ -51,7 +51,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, > data->obj = qobject_from_jsonv(json_string, ap); > g_assert(data->obj); > > - data->qiv = qmp_input_visitor_new(data->obj); > + data->qiv = qmp_input_visitor_new(data->obj, false); > g_assert(data->qiv); > > v = qmp_input_get_visitor(data->qiv); > diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c Stay lenient (because we're testing it). > index 9adbc30..7b14b5a 100644 > --- a/tests/test-visitor-serialization.c > +++ b/tests/test-visitor-serialization.c > @@ -1038,7 +1038,7 @@ static void qmp_deserialize(void **native_out, void *datap, > obj = qobject_from_json(qstring_get_str(output_json)); > > QDECREF(output_json); > - d->qiv = qmp_input_visitor_new(obj); > + d->qiv = qmp_input_visitor_new(obj, true); > qobject_decref(obj_orig); > qobject_decref(obj); > visit(qmp_input_get_visitor(d->qiv), native_out, errp); Switch to strict. Not mentioned in commit message. Why is it a good idea? > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 0d53691..2a2c524 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -1145,7 +1145,7 @@ void qapi_copy_SocketAddress(SocketAddress **p_dest, > return; > } > > - qiv = qmp_input_visitor_new(obj); > + qiv = qmp_input_visitor_new(obj, true); > iv = qmp_input_get_visitor(qiv); > visit_type_SocketAddress(iv, NULL, p_dest, &error_abort); > qmp_input_visitor_cleanup(qiv); Switch to strict. The commit message explains why this is a good idea. > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index 0e4baff..4a917f9 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -996,7 +996,7 @@ Example: > { > Error *err = NULL; > UserDefOne *retval; > - QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); > + QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); > QapiDeallocVisitor *qdv; > Visitor *v; > UserDefOneList *arg1 = NULL; Stay strict.
On 04/28/2016 07:06 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Rather than having two separate ways to create a QMP input >> visitor, where the safer approach has the more verbose name, >> it is better to consolidate things into a single function >> where the caller must explicitly choose whether to be strict >> or to ignore excess input. Use strict mode in more places of >> internal code (such as when cloning a QAPI struct in >> util/socket.c, where the QObject better not have excess >> input since it was just generated by qmp-output), while >> documenting in user-facing code a question of whether we >> should change our policy about ignoring excess input. > > Which external interface is this? QMP qom-set, via object_property_set_qobject() > >> >> In the case of qmp_object_add(), we intentionally switch to a >> strict visitor; this matches the fact that the code for >> user_creatable_add_type() is shared by both qmp_object_add() >> (QMP input visitor) and by user_creatable_add_opts() (QemuOpts >> visitor); the latter is always strict, so our usage in the >> former should also be strict, so that both visits will >> equally diagnose any excess input in a nested dict. But in >> practice, we don't really have any -object where the >> properties are a nested dict, and excess input at the top >> level is already caught earlier by object_property_set() on >> an unknown key, whether from QemuOpts: >> >> $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio -object secret,id=sec0,data=letmein,format=raw,foo=bar >> qemu-system-x86_64: Property '.foo' not found > > Let's update the error message now that the error message regression is > fixed. Can do on commit. > Thanks. And now you know why I reported the regression. >> >> or from QMP: >> >> $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio >> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 5, "major": 2}, "package": ""}, "capabilities": []}} >> {"execute":"qmp_capabilities"} >> {"return": {}} >> {"execute":"object-add","arguments":{"qom-type":"secret","id":"sec0","props":{"format":"raw","data":"letmein","a":1}}} >> {"error": {"class": "GenericError", "desc": "Property '.a' not found"}} >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Should we split this into a patch to change the interface, and one or > more separate patches to switch to the strict visitor? Could result in > clearer and more complete commit messages. Could do. >> +++ b/qom/qom-qobject.c >> @@ -22,7 +22,8 @@ void object_property_set_qobject(Object *obj, QObject *value, >> const char *name, Error **errp) >> { >> QmpInputVisitor *qiv; >> - qiv = qmp_input_visitor_new(value); >> + /* TODO: Should we reject, rather than ignore, excess input? */ >> + qiv = qmp_input_visitor_new(value, false); >> object_property_set(obj, qmp_input_get_visitor(qiv), name, errp); >> >> qmp_input_visitor_cleanup(qiv); > > Stay lenient, but document this should perhaps switch to strict. The > commit message hints at this one. "hints at" and "explicitly mentions by name" are two different things, I get your point that my commit message could have been better. >> +++ b/replay/replay-input.c >> @@ -37,7 +37,7 @@ static InputEvent *qapi_clone_InputEvent(InputEvent *src) >> return NULL; >> } >> >> - qiv = qmp_input_visitor_new(obj); >> + qiv = qmp_input_visitor_new(obj, true); >> iv = qmp_input_get_visitor(qiv); >> visit_type_InputEvent(iv, NULL, &dst, &error_abort); >> qmp_input_visitor_cleanup(qiv); > > Switch to strict. Not mentioned in commit message. Why is it a good > idea? Same category as util/qemu-sockets.c: when cloning, we shouldn't be introducing any junk from the QObject just produced from the qmp-output visitor. (Hmm, that also means that when cloning, we are now doing MORE work to check for a "can't happen" condition - but it all goes away in a later patch when I introduce a real cloning visitor that is more efficient than a round-trip qmp-output => qmp-input double pass - so maybe on that grounds, I don't need to convert this or the qemu-sockets.c case) >> +++ b/tests/test-qmp-commands.c >> @@ -222,7 +222,7 @@ static void test_dealloc_partial(void) >> ud2_dict = qdict_new(); >> qdict_put_obj(ud2_dict, "string0", QOBJECT(qstring_from_str(text))); >> >> - qiv = qmp_input_visitor_new(QOBJECT(ud2_dict)); >> + qiv = qmp_input_visitor_new(QOBJECT(ud2_dict), true); >> visit_type_UserDefTwo(qmp_input_get_visitor(qiv), NULL, &ud2, &err); >> qmp_input_visitor_cleanup(qiv); >> QDECREF(ud2_dict); > > Switch to strict. Not mentioned in commit message. Why is it a good > idea? Because it's a testsuite and still passes with stricter testing, thus any future additions to the testsuite will exercise strict mode which is what we want to use in more places. >> +++ b/tests/test-qmp-input-visitor.c >> @@ -51,7 +51,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, >> data->obj = qobject_from_jsonv(json_string, ap); >> g_assert(data->obj); >> >> - data->qiv = qmp_input_visitor_new(data->obj); >> + data->qiv = qmp_input_visitor_new(data->obj, false); >> g_assert(data->qiv); >> >> v = qmp_input_get_visitor(data->qiv); >> diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c > > Stay lenient (because we're testing it). If we get rid of all lenient clients other than test-qmp-input-visitor, then maybe lenient mode is worth nuking, and simplify our testsuite by consolidating test-qmp-input-{visitor,strict}? But food for thought for a later day. >> +++ b/tests/test-visitor-serialization.c >> @@ -1038,7 +1038,7 @@ static void qmp_deserialize(void **native_out, void *datap, >> obj = qobject_from_json(qstring_get_str(output_json)); >> >> QDECREF(output_json); >> - d->qiv = qmp_input_visitor_new(obj); >> + d->qiv = qmp_input_visitor_new(obj, true); >> qobject_decref(obj_orig); >> qobject_decref(obj); >> visit(qmp_input_get_visitor(d->qiv), native_out, errp); > > Switch to strict. Not mentioned in commit message. Why is it a good > idea? Same argument as test-qmp-commands, and same flaw in the commit message for not calling it out.
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index b570069..6261e44 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -115,7 +115,7 @@ def gen_marshal(name, arg_type, ret_type): if arg_type and arg_type.members: ret += mcgen(''' - QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); + QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); QapiDeallocVisitor *qdv; Visitor *v; %(c_name)s arg = {0}; diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h index 3ed499c..b0624d8 100644 --- a/include/qapi/qmp-input-visitor.h +++ b/include/qapi/qmp-input-visitor.h @@ -19,8 +19,13 @@ typedef struct QmpInputVisitor QmpInputVisitor; -QmpInputVisitor *qmp_input_visitor_new(QObject *obj); -QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj); +/* + * Return a new input visitor that converts QMP to QAPI. + * + * Set @strict to reject a parse that doesn't consume all keys of a + * dictionary; otherwise excess input is ignored. + */ +QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool strict); void qmp_input_visitor_cleanup(QmpInputVisitor *v); diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 8550bc7..c3c3271 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -356,7 +356,7 @@ void qmp_input_visitor_cleanup(QmpInputVisitor *v) g_free(v); } -QmpInputVisitor *qmp_input_visitor_new(QObject *obj) +QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool strict) { QmpInputVisitor *v; @@ -376,19 +376,10 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) v->visitor.type_number = qmp_input_type_number; v->visitor.type_any = qmp_input_type_any; v->visitor.optional = qmp_input_optional; + v->strict = strict; qmp_input_push(v, obj, NULL); qobject_incref(obj); return v; } - -QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj) -{ - QmpInputVisitor *v; - - v = qmp_input_visitor_new(obj); - v->strict = true; - - return v; -} diff --git a/qmp.c b/qmp.c index 9d0953b..e784a67 100644 --- a/qmp.c +++ b/qmp.c @@ -663,7 +663,7 @@ void qmp_object_add(const char *type, const char *id, } } - qiv = qmp_input_visitor_new(props); + qiv = qmp_input_visitor_new(props, true); obj = user_creatable_add_type(type, id, pdict, qmp_input_get_visitor(qiv), errp); qmp_input_visitor_cleanup(qiv); diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c index e6b17c1..b66088d 100644 --- a/qom/qom-qobject.c +++ b/qom/qom-qobject.c @@ -22,7 +22,8 @@ void object_property_set_qobject(Object *obj, QObject *value, const char *name, Error **errp) { QmpInputVisitor *qiv; - qiv = qmp_input_visitor_new(value); + /* TODO: Should we reject, rather than ignore, excess input? */ + qiv = qmp_input_visitor_new(value, false); object_property_set(obj, qmp_input_get_visitor(qiv), name, errp); qmp_input_visitor_cleanup(qiv); diff --git a/replay/replay-input.c b/replay/replay-input.c index 06babe0..03e99d5 100644 --- a/replay/replay-input.c +++ b/replay/replay-input.c @@ -37,7 +37,7 @@ static InputEvent *qapi_clone_InputEvent(InputEvent *src) return NULL; } - qiv = qmp_input_visitor_new(obj); + qiv = qmp_input_visitor_new(obj, true); iv = qmp_input_get_visitor(qiv); visit_type_InputEvent(iv, NULL, &dst, &error_abort); qmp_input_visitor_cleanup(qiv); diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index 14a9ebb..597fb44 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -222,7 +222,7 @@ static void test_dealloc_partial(void) ud2_dict = qdict_new(); qdict_put_obj(ud2_dict, "string0", QOBJECT(qstring_from_str(text))); - qiv = qmp_input_visitor_new(QOBJECT(ud2_dict)); + qiv = qmp_input_visitor_new(QOBJECT(ud2_dict), true); visit_type_UserDefTwo(qmp_input_get_visitor(qiv), NULL, &ud2, &err); qmp_input_visitor_cleanup(qiv); QDECREF(ud2_dict); diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index d5f80ec..2b053a2 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -55,7 +55,7 @@ static Visitor *validate_test_init_internal(TestInputVisitorData *data, data->obj = qobject_from_jsonv(json_string, ap); g_assert(data->obj); - data->qiv = qmp_input_visitor_new_strict(data->obj); + data->qiv = qmp_input_visitor_new(data->obj, true); g_assert(data->qiv); v = qmp_input_get_visitor(data->qiv); diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 80527eb..c039806 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -51,7 +51,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, data->obj = qobject_from_jsonv(json_string, ap); g_assert(data->obj); - data->qiv = qmp_input_visitor_new(data->obj); + data->qiv = qmp_input_visitor_new(data->obj, false); g_assert(data->qiv); v = qmp_input_get_visitor(data->qiv); diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index 9adbc30..7b14b5a 100644 --- a/tests/test-visitor-serialization.c +++ b/tests/test-visitor-serialization.c @@ -1038,7 +1038,7 @@ static void qmp_deserialize(void **native_out, void *datap, obj = qobject_from_json(qstring_get_str(output_json)); QDECREF(output_json); - d->qiv = qmp_input_visitor_new(obj); + d->qiv = qmp_input_visitor_new(obj, true); qobject_decref(obj_orig); qobject_decref(obj); visit(qmp_input_get_visitor(d->qiv), native_out, errp); diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 0d53691..2a2c524 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1145,7 +1145,7 @@ void qapi_copy_SocketAddress(SocketAddress **p_dest, return; } - qiv = qmp_input_visitor_new(obj); + qiv = qmp_input_visitor_new(obj, true); iv = qmp_input_get_visitor(qiv); visit_type_SocketAddress(iv, NULL, p_dest, &error_abort); qmp_input_visitor_cleanup(qiv); diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 0e4baff..4a917f9 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -996,7 +996,7 @@ Example: { Error *err = NULL; UserDefOne *retval; - QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); + QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); QapiDeallocVisitor *qdv; Visitor *v; UserDefOneList *arg1 = NULL;
Rather than having two separate ways to create a QMP input visitor, where the safer approach has the more verbose name, it is better to consolidate things into a single function where the caller must explicitly choose whether to be strict or to ignore excess input. Use strict mode in more places of internal code (such as when cloning a QAPI struct in util/socket.c, where the QObject better not have excess input since it was just generated by qmp-output), while documenting in user-facing code a question of whether we should change our policy about ignoring excess input. In the case of qmp_object_add(), we intentionally switch to a strict visitor; this matches the fact that the code for user_creatable_add_type() is shared by both qmp_object_add() (QMP input visitor) and by user_creatable_add_opts() (QemuOpts visitor); the latter is always strict, so our usage in the former should also be strict, so that both visits will equally diagnose any excess input in a nested dict. But in practice, we don't really have any -object where the properties are a nested dict, and excess input at the top level is already caught earlier by object_property_set() on an unknown key, whether from QemuOpts: $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio -object secret,id=sec0,data=letmein,format=raw,foo=bar qemu-system-x86_64: Property '.foo' not found or from QMP: $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio {"QMP": {"version": {"qemu": {"micro": 93, "minor": 5, "major": 2}, "package": ""}, "capabilities": []}} {"execute":"qmp_capabilities"} {"return": {}} {"execute":"object-add","arguments":{"qom-type":"secret","id":"sec0","props":{"format":"raw","data":"letmein","a":1}}} {"error": {"class": "GenericError", "desc": "Property '.a' not found"}} Signed-off-by: Eric Blake <eblake@redhat.com> --- v15: new patch --- scripts/qapi-commands.py | 2 +- include/qapi/qmp-input-visitor.h | 9 +++++++-- qapi/qmp-input-visitor.c | 13 ++----------- qmp.c | 2 +- qom/qom-qobject.c | 3 ++- replay/replay-input.c | 2 +- tests/test-qmp-commands.c | 2 +- tests/test-qmp-input-strict.c | 2 +- tests/test-qmp-input-visitor.c | 2 +- tests/test-visitor-serialization.c | 2 +- util/qemu-sockets.c | 2 +- docs/qapi-code-gen.txt | 2 +- 12 files changed, 20 insertions(+), 23 deletions(-)