Re: [PATCH v2 19/31] qapi/qom: QAPIfy object-add

[Kevin Wolf <kwolf@redhat.com>]

Am 26.02.2021 um 22:18 hat Eric Blake geschrieben:
> On 2/24/21 7:52 AM, Kevin Wolf wrote:
> > This converts object-add from 'gen': false to the ObjectOptions QAPI
> > type. As an immediate benefit, clients can now use QAPI schema
> > introspection for user creatable QOM objects.
> > 
> > It is also the first step towards making the QAPI schema the only
> > external interface for the creation of user creatable objects. Once all
> > other places (HMP and command lines of the system emulator and all
> > tools) go through QAPI, too, some object implementations can be
> > simplified because some checks (e.g. that mandatory options are set) are
> > already performed by QAPI, and in another step, QOM boilerplate code
> > could be generated from the schema.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/qom.json                        | 11 +----------
> >  include/qom/object_interfaces.h      |  7 -------
> >  hw/block/xen-block.c                 | 16 ++++++++--------
> >  monitor/misc.c                       |  2 --
> >  qom/qom-qmp-cmds.c                   | 25 +++++++++++++++++++++++--
> >  storage-daemon/qemu-storage-daemon.c |  2 --
> >  6 files changed, 32 insertions(+), 31 deletions(-)
> > 
> > +++ b/qapi/qom.json
> > @@ -839,13 +839,6 @@
> >  #
> >  # Create a QOM object.
> >  #
> > -# @qom-type: the class name for the object to be created
> > -#
> > -# @id: the name of the new object
> > -#
> > -# Additional arguments depend on qom-type and are passed to the backend
> > -# unchanged.
> > -#
> >  # Returns: Nothing on success
> >  #          Error if @qom-type is not a valid class name
> >  #
> > @@ -859,9 +852,7 @@
> >  # <- { "return": {} }
> >  #
> >  ##
> > -{ 'command': 'object-add',
> > -  'data': {'qom-type': 'str', 'id': 'str'},
> > -  'gen': false } # so we can get the additional arguments
> > +{ 'command': 'object-add', 'data': 'ObjectOptions', 'boxed': true }
> 
> So much more concise ;)  A grep for TYPE_USER_CREATABLE doesn't seem to
> turn up any *_class_init() functions that your earlier patches in the
> series missed, so I think you captured an accurate 1:1 mapping.  There
> is include/chardev/char.h with the comment about "TODO: eventually use
> TYPE_USER_CREATABLE" which may point to the next item to be added to
> ObjectOptions, but that's not for this series.
> 
> > +++ b/qom/qom-qmp-cmds.c
> 
> >  
> > -void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
> > +void qmp_object_add(ObjectOptions *options, Error **errp)
> >  {
> > -    user_creatable_add_dict(qdict, false, errp);
> > +    Visitor *v;
> > +    QObject *qobj;
> > +    QDict *props;
> > +    Object *obj;
> > +
> > +    v = qobject_output_visitor_new(&qobj);
> > +    visit_type_ObjectOptions(v, NULL, &options, &error_abort);
> > +    visit_complete(v, &qobj);
> > +    visit_free(v);
> 
> This part is nice...

It's not really, though.

We're going from ObjectOptions to QDict just to feed the QDict back into
a visitor. The QDict step feels unnecessary, but we don't have a visitor
that visits existing QAPI objects. I think it would be somewhat similar
to the clone visitor, but not exactly the same thing.

> > +
> > +    props = qobject_to(QDict, qobj);
> > +    qdict_del(props, "qom-type");
> > +    qdict_del(props, "id");
> 
> ...while this part makes it seem like we still have more cleanup to come
> later.  But hey, progress!

Ideally, I would like the whole function to look more or less like this:

void qmp_object_add(ObjectOptions *options, Error **errp)
{
    Visitor *v = qapi_object_visitor_new(options);
    Object *obj = user_creatable_add_type(v, errp);
    object_unref(obj);
    visit_free(v);
}

Can be done later (or never).

Kevin

> > +
> > +    v = qobject_input_visitor_new(QOBJECT(props));
> > +    obj = user_creatable_add_type(ObjectType_str(options->qom_type),
> > +                                  options->id, props, v, errp);
> > +    object_unref(obj);
> > +    visit_free(v);
> >  }
> >  
> 
> Once you address Paolo's comment, you can also add
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org