Re: [RFC PATCH 02/12] qom: Create object_configure()

[Kevin Wolf]

Am 23.11.2021 um 16:23 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This renames object_set_properties_from_qdict() to object_configure()
> > and removes the QDict parameter from it: With visit_next_struct_member()
> > it can set all properties without looking at the keys of the QDict.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qom/object_interfaces.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index 3b61c195c5..f9f5608194 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -42,16 +42,15 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
> >      }
> >  }
> >  
> > -static void object_set_properties_from_qdict(Object *obj, const QDict 
> > *qdict,
> > -                                             Visitor *v, Error **errp)
> > +static void object_configure(Object *obj, Visitor *v, Error **errp)
> >  {
> > -    const QDictEntry *e;
> > +    const char *key;
> >  
> >      if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
> >          return;
> >      }
> > -    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> > -        if (!object_property_set(obj, e->key, v, errp)) {
> > +    while ((key = visit_next_struct_member(v))) {
> > +        if (!object_property_set(obj, key, v, errp)) {
> >              goto out;
> >          }
> >      }
> > @@ -69,7 +68,7 @@ void object_set_properties_from_keyval(Object *obj, const 
> > QDict *qdict,
> >      } else {
> >          v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> >      }
> > -    object_set_properties_from_qdict(obj, qdict, v, errp);
> > +    object_configure(obj, v, errp);
> >      visit_free(v);
> >  }
> >  
> > @@ -108,7 +107,7 @@ Object *user_creatable_add_type(const char *type, const 
> > char *id,
> >  
> >      assert(qdict);
> 
> This is the only remaining use of parameter @qdict.  Let's drop it.

Indeed, very good. I've never liked this interface that needs a QDict in
addition to the visitor.

> >      obj = object_new(type);
> > -    object_set_properties_from_qdict(obj, qdict, v, &local_err);
> > +    object_configure(obj, v, &local_err);
> >      if (local_err) {
> >          goto out;
> >      }
> 
> Brief recap how configuration data flows trough QMP object-add to QOM:
> 
> * QMP object-add is fully typed: union ObjectOptions.  QMP core parses
>   input via QDict to ObjectOptions (JSON parser + QObject input
>   visitor), and passes that to qmp_object_add().
> 
> * qmp_object_add() passes it on to user_creatable_add_qapi().
> 
>   Aside: getting rid of the wrapper would be as easy as renaming
>   user_creatable_add_qapi() to qmp_object_add().
> 
> * user_creatable_add_qapi() converts right back to QDict (QObject output
>   visitor).  It extracts the non-properties "qom-type" and "id", and
>   passes the properties (wrapped in a QObject input visitor) to
>   user_creatable_add_type().
> 
> * user_creatable_add_type() feeds the properties to object_configure(),
>   formerly object_set_properties_from_qdict().
> 
> Your patch simplifies one detail of the last step.  Small
> simplifications are welcome, too.
> 
> The visitor ping-pong remains: input -> output -> input.
> 
> We play visitor ping-pong because we reduce the problem "for each member
> of ObjectOptions: set property" to the solved problem "set properties
> for an input visitor".
> 
> Straight solution of the problem: a QOM property output visitor.

I'm not sure how that should work? You can't drive a visit from both
sides and QOM properties already take an input visitor. The only way I
see to make this work is converting to QObjects and creating input
visitors internally in the QOM property output visitor, which would kind
of defeat the purpose.

What could in theory be done is passing a Visitor to qmp_object_add()
instead of ObjectOptions (in the long run, we probably don't want a big
union of all existing QOM types anyway). But if directly pass this on to
QOM, we lose all of the QAPI validation, which we don't want either.

Kevin