Re: [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts

[Laurent Vivier]

On 22/03/2017 15:45, Eric Blake wrote:
> 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 (this does mean that once instantiated,
> QemuOpts is not necessarily an accurate representation of the
> original command line - but this is not the first place to do that);
> however "id" has to be put back (requiring us to cast away a const).
> 
> [As a side note, hmp_object_add() turns a QDict into a QemuOpts,
> then calls user_creatable_add_opts() which converts QemuOpts into
> a new QDict. There are probably a lot of wasteful conversions like
> this, but cleaning them up is a much bigger task than the immediate
> regression fix.]
> 
> CC: address@hidden
> Signed-off-by: Eric Blake <address@hidden>

Tested-by: Laurent Vivier <address@hidden>

> 
> ---
> v3: enhance commit message
> 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;
>  }
>