| Message ID | 20210224135255.253837-20-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi/qom: QAPIfy --object and object-add | expand |
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
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... > + > + 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 <eblake@redhat.com>
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
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
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
diff --git a/qapi/qom.json b/qapi/qom.json index 6793342e81..e5b219df58 100644 --- a/qapi/qom.json +++ 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 } ## # @object-del: diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h index 07d5cc8832..9b9938b8c0 100644 --- a/include/qom/object_interfaces.h +++ b/include/qom/object_interfaces.h @@ -196,11 +196,4 @@ bool user_creatable_del(const char *id, Error **errp); */ void user_creatable_cleanup(void); -/** - * qmp_object_add: - * - * QMP command handler for object-add. See the QAPI schema for documentation. - */ -void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp); - #endif diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index a3b69e2709..ac82d54063 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -836,17 +836,17 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id, { ERRP_GUARD(); XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1); - QDict *opts; - QObject *ret_data = NULL; + ObjectOptions *opts; iothread->id = g_strdup(id); - opts = qdict_new(); - qdict_put_str(opts, "qom-type", TYPE_IOTHREAD); - qdict_put_str(opts, "id", id); - qmp_object_add(opts, &ret_data, errp); - qobject_unref(opts); - qobject_unref(ret_data); + opts = g_new(ObjectOptions, 1); + *opts = (ObjectOptions) { + .qom_type = OBJECT_TYPE_IOTHREAD, + .id = g_strdup(id), + }; + qmp_object_add(opts, errp); + qapi_free_ObjectOptions(opts); if (*errp) { g_free(iothread->id); diff --git a/monitor/misc.c b/monitor/misc.c index a7650ed747..42efd9e2ab 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -235,8 +235,6 @@ static void monitor_init_qmp_commands(void) qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG); qmp_register_command(&qmp_commands, "device_add", qmp_device_add, QCO_NO_OPTIONS); - qmp_register_command(&qmp_commands, "object-add", qmp_object_add, - QCO_NO_OPTIONS); QTAILQ_INIT(&qmp_cap_negotiation_commands); qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities", diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c index 19fd5e117f..e577a96adf 100644 --- a/qom/qom-qmp-cmds.c +++ b/qom/qom-qmp-cmds.c @@ -19,8 +19,11 @@ #include "qapi/error.h" #include "qapi/qapi-commands-qdev.h" #include "qapi/qapi-commands-qom.h" +#include "qapi/qapi-visit-qom.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qobject-output-visitor.h" #include "qemu/cutils.h" #include "qom/object_interfaces.h" #include "qom/qom-qobject.h" @@ -223,9 +226,27 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, return prop_list; } -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); + + 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); + visit_free(v); } void qmp_object_del(const char *id, Error **errp) diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index 9021a46b3a..d8d172cc60 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -144,8 +144,6 @@ static void init_qmp_commands(void) qmp_init_marshal(&qmp_commands); qmp_register_command(&qmp_commands, "query-qmp-schema", qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG); - qmp_register_command(&qmp_commands, "object-add", qmp_object_add, - QCO_NO_OPTIONS); QTAILQ_INIT(&qmp_cap_negotiation_commands); qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
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(-)