Re: [Qemu-devel] [PATCH v15 09/23] qom: Wrap prop visit in visit_start_struct

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The qmp-input visitor was allowing callers to play rather fast
> and loose: when visiting a QDict, you could grab members of the
> root dictionary without first pushing into the dict; the final
> such culprit was the QOM code for converting to and from object
> properties.  But we are about to tighten the input visitor, at
> which point user_creatable_add_type() as called with a QMP input
> visitor via qmp_object_add() MUST follow the same paradigms as
> everyone else, of pushing into the struct before grabbing its
> keys.
>
> The use of 'err ? NULL : &err' is temporary; a later patch will
> clean that up when it splits visit_end_struct().
>
> The change has no impact to the testsuite now, but is required to
> avoid a failure in tests/test-netfilter once qmp-input is made
> stricter to detect inconsistent 'name' arguments on the root visit.
>
> Since user_creatable_add_type() is also called with OptsVisitor
> through user_creatable_add_opts(), we must also check that there
> is no negative impact there; both pre- and post-patch, we see:
>
> $ ./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.

> That is, the only new checking that the new visit_end_struct() can
> perform is for excess input, but we already catch excess input
> earlier in object_property_set().

Answers the question I had for v14.  Didn't double check myself; I trust
your analysis.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v15: hoist earlier in series, improve commit message
> v14: no change
> v13: no change
> v12: new patch
> ---
>  qom/object_interfaces.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index ab5da35..4a60d6d 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -120,12 +120,20 @@ Object *user_creatable_add_type(const char *type, const 
> char *id,
>
>      obj = object_new(type);
>      if (qdict) {
> +        visit_start_struct(v, NULL, NULL, 0, &local_err);
> +        if (local_err) {
> +            goto out;
> +        }
>          for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
>              object_property_set(obj, v, e->key, &local_err);
>              if (local_err) {
> -                goto out;
> +                break;
>              }
>          }
> +        visit_end_struct(v, local_err ? NULL : &local_err);
> +        if (local_err) {
> +            goto out;
> +        }
>      }
>
>      object_property_add_child(object_get_objects_root(),

Hmm.  How should this behave if !qdict?

Compare:

       visit_start_struct(v, NULL, NULL, 0, &local_err);
       if (local_err) {
           goto out;
       }
       if (qdict) {
           for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
               object_property_set(obj, v, e->key, &local_err);
               if (local_err) {
                   break;
               }
           }
       }
       visit_end_struct(v, local_err ? NULL : &local_err);
       if (local_err) {
           goto out;
       }

The difference is that we get errors from visit_start_struct() and
visit_end_struct() even when !qdict.

However, both callers seem to pass non-null qdict.  Should we sidestep
the question by making that a precondition?