[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/3] qom: Avoid unvisited 'id'/'qom-type' in
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts |
Date: |
Wed, 22 Mar 2017 07:42:24 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> A regression in commit 15c2f669e caused us to silently ignore
> excess input to the QemuOpts visitor. Later, commit ea4641
> accidentally abused that situation, by removing "qom-type" and
> "id" from the corresponding QDict but leaving them defined in
> the QemuOpts, when using the pair of containers to create a
> user-defined object. Note that since we are already traversing
> two separate items (a QDict and a QemuOpts), we are already
> able to flag bogus arguments, as in:
>
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio
> -object memory-backend-ram,id=mem1,size=4k,bogus=huh
> qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh:
> Property '.bogus' not found
>
> So the only real concern is that when we re-enable strict checking
> in the QemuOpts visitor, we do not want to start flagging the two
> leftover keys as unvisited. Rearrange the code to clean out the
> QemuOpts listing in advance, rather than removing items from the
> QDict. Since "qom-type" is usually an automatic implicit default,
> we don't have to restore it; but "id" has to be put back (requiring
> us to cast away a const).
This is a yet another example of how actual configuration can easily
diverge from the one in QemuOpts. Discussed recently in:
Subject: Re: [PATCH 0/2] add writeconfig command on monitor
Message-ID: <address@hidden>
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03476.html
Not putting "qom-type" back here is okay. Putting it back would also be
okay. I guess what you prefer depends on your level of OCD.
> CC: address@hidden
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v2: new patch
> ---
> qom/object_interfaces.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 03a95c3..cc9a694 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -114,7 +114,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error
> **errp)
> QDict *pdict;
> Object *obj;
> const char *id = qemu_opts_id(opts);
> - const char *type = qemu_opt_get(opts, "qom-type");
> + char *type = qemu_opt_get_del(opts, "qom-type");
>
> if (!type) {
> error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
> @@ -125,14 +125,15 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error
> **errp)
> return NULL;
> }
>
> + qemu_opts_set_id(opts, NULL);
> pdict = qemu_opts_to_qdict(opts, NULL);
> - qdict_del(pdict, "qom-type");
> - qdict_del(pdict, "id");
>
> v = opts_visitor_new(opts);
> obj = user_creatable_add_type(type, id, pdict, v, errp);
> visit_free(v);
>
> + qemu_opts_set_id(opts, (char *) id);
> + g_free(type);
> QDECREF(pdict);
> return obj;
> }
Aside: I dislike how this converts QemuOpts to QDict so
user_creatable_add_type() can use the QDict to guide the visit.
Awkward, as so much code that uses QemuOpts in not entirely
straightforward ways.