[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v15 05/23] qapi: Use strict QMP input visitor in
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v15 05/23] qapi: Use strict QMP input visitor in more places |
Date: |
Thu, 28 Apr 2016 15:06:39 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
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.
- Re: [Qemu-devel] [PATCH v15 11/23] qmp-input: Refactor when list is advanced, (continued)
- [Qemu-devel] [PATCH v15 14/23] qapi: Add visit_type_null() visitor, Eric Blake, 2016/04/27
- [Qemu-devel] [PATCH v15 05/23] qapi: Use strict QMP input visitor in more places, Eric Blake, 2016/04/27
- Re: [Qemu-devel] [PATCH v15 05/23] qapi: Use strict QMP input visitor in more places,
Markus Armbruster <=
- [Qemu-devel] [PATCH v15 08/23] monitor: Let generated code validate arguments, Eric Blake, 2016/04/27
- Re: [Qemu-devel] [PATCH v15 08/23] monitor: Let generated code validate arguments, Eric Blake, 2016/04/28
[Qemu-devel] [PATCH v15 01/23] qapi-visit: Add visitor.type classification, Eric Blake, 2016/04/27
[Qemu-devel] [PATCH v15 10/23] qmp-input: Require struct push to visit members of top dict, Eric Blake, 2016/04/27