Re: [PATCH v2 19/31] qapi/qom: QAPIfy object-add
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
> > ---
> > 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
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 19/31] qapi/qom: QAPIfy object-add
On 01/03/21 12:54, Kevin Wolf wrote: Please add a check in object_property_add_child that the id is well formed (using the id_wellformed function). This is pre-existing, but it becomes a regression for -object later in the series. Are the conditions for internally called object_property_add_child() actually the same as for IDs specified by the user? For example, I seem to remember some array-ish properties with [] in their name which aren't allowed by id_wellformed(). Yes, you are right. The obvious place to affect only the external interfaces would be user_creatable_add_type(). Makes sense, thanks. Paolo
Re: [PATCH v2 19/31] qapi/qom: QAPIfy object-add
Am 26.02.2021 um 12:30 hat Paolo Bonzini geschrieben: > On 24/02/21 14:52, Kevin Wolf wrote: > > +v = qobject_output_visitor_new(&qobj); > > +visit_type_ObjectOptions(v, NULL, &options, &error_abort); > > +visit_complete(v, &qobj); > > +visit_free(v); > > + > > +props = qobject_to(QDict, qobj); > > +qdict_del(props, "qom-type"); > > +qdict_del(props, "id"); > > + > > +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); > > Please add a check in object_property_add_child that the id is well formed > (using the id_wellformed function). This is pre-existing, but it becomes a > regression for -object later in the series. Are the conditions for internally called object_property_add_child() actually the same as for IDs specified by the user? For example, I seem to remember some array-ish properties with [] in their name which aren't allowed by id_wellformed(). The obvious place to affect only the external interfaces would be user_creatable_add_type(). Kevin
Re: [PATCH v2 19/31] qapi/qom: QAPIfy object-add
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
> ---
> 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...
> +
> +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!
> +
> +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
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 19/31] qapi/qom: QAPIfy object-add
On 24/02/21 14:52, Kevin Wolf wrote: +v = qobject_output_visitor_new(&qobj); +visit_type_ObjectOptions(v, NULL, &options, &error_abort); +visit_complete(v, &qobj); +visit_free(v); + +props = qobject_to(QDict, qobj); +qdict_del(props, "qom-type"); +qdict_del(props, "id"); + +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); Please add a check in object_property_add_child that the id is well formed (using the id_wellformed function). This is pre-existing, but it becomes a regression for -object later in the series. Thanks, Paolo
