| Message ID | 20200306171458.1848-14-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/29] qcow2: Fix alloc_cluster_abort() for pre-existing clusters | expand |
On 06/03/20 18:14, Kevin Wolf wrote: > Mapping object-add to the command line as is doesn't result in nice > syntax because of the nesting introduced with 'props'. This becomes > nicer and more consistent with device_add and netdev_add when we accept > properties for the object on the top level instead. > > 'props' is still accepted after this patch, but marked as deprecated. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20200224143008.13362-8-kwolf@redhat.com> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Hi Kevin and Markus, I just noticed this patch. In my opinion "nice syntax" is not a good argument for having a "gen: false" command that is even less introspectable than the "props" argument. As an aside, it would have been nice to run this through Markus and me, though in all fairness I'm not sure I would have been responsive back in February. I would like to un-deprecate this for 5.1, and revert it in either 5.1 or 5.2. (Also I will be away next week, so the decision would have to be taken quickly). Paolo > --- > qapi/qom.json | 12 +++++++--- > docs/system/deprecated.rst | 5 ++++ > include/qom/object_interfaces.h | 7 ++++++ > hw/block/xen-block.c | 11 ++++++++- > monitor/misc.c | 2 ++ > qom/qom-qmp-cmds.c | 42 +++++++++++++++++++++++++++------ > 6 files changed, 68 insertions(+), 11 deletions(-) > > diff --git a/qapi/qom.json b/qapi/qom.json > index ecc60c4401..8abe998962 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -210,7 +210,12 @@ > # > # @id: the name of the new object > # > -# @props: a dictionary of properties to be passed to the backend > +# @props: a dictionary of properties to be passed to the backend. Deprecated > +# since 5.0, specify the properties on the top level instead. It is an > +# error to specify the same option both on the top level and in @props. > +# > +# 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 > @@ -221,12 +226,13 @@ > # > # -> { "execute": "object-add", > # "arguments": { "qom-type": "rng-random", "id": "rng1", > -# "props": { "filename": "/dev/hwrng" } } } > +# "filename": "/dev/hwrng" } } > # <- { "return": {} } > # > ## > { 'command': 'object-add', > - 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} } > + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'}, > + 'gen': false } # so we can get the additional arguments > > ## > # @object-del: > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index 1eaa559079..6c1d9034d9 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -190,6 +190,11 @@ Use ``migrate-set-parameters`` instead. > > Use ``migrate-set-parameters`` and ``query-migrate-parameters`` instead. > > +``object-add`` option ``props`` (since 5.0) > +''''''''''''''''''''''''''''''''''''''''''' > + > +Specify the properties for the object as top-level arguments instead. > + > ``query-block`` result field ``dirty-bitmaps[i].status`` (since 4.0) > '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h > index 3e4e1d928b..6f92f3cebb 100644 > --- a/include/qom/object_interfaces.h > +++ b/include/qom/object_interfaces.h > @@ -162,4 +162,11 @@ void 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 686bbc3f0d..3885464513 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -18,6 +18,7 @@ > #include "qapi/visitor.h" > #include "qapi/qmp/qdict.h" > #include "qapi/qmp/qstring.h" > +#include "qom/object_interfaces.h" > #include "hw/xen/xen_common.h" > #include "hw/block/xen_blkif.h" > #include "hw/qdev-properties.h" > @@ -858,10 +859,18 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id, > { > XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1); > Error *local_err = NULL; > + QDict *opts; > + QObject *ret_data; > > iothread->id = g_strdup(id); > > - qmp_object_add(TYPE_IOTHREAD, id, false, NULL, &local_err); > + opts = qdict_new(); > + qdict_put_str(opts, "qom-type", TYPE_IOTHREAD); > + qdict_put_str(opts, "id", id); > + qmp_object_add(opts, &ret_data, &local_err); > + qobject_unref(opts); > + qobject_unref(ret_data); > + > if (local_err) { > error_propagate(errp, local_err); > > diff --git a/monitor/misc.c b/monitor/misc.c > index 6c41293102..1748ab3911 100644 > --- a/monitor/misc.c > +++ b/monitor/misc.c > @@ -248,6 +248,8 @@ static void monitor_init_qmp_commands(void) > QCO_NO_OPTIONS); > qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_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 6136efec16..49db926fcc 100644 > --- a/qom/qom-qmp-cmds.c > +++ b/qom/qom-qmp-cmds.c > @@ -14,6 +14,7 @@ > */ > > #include "qemu/osdep.h" > +#include "block/qdict.h" > #include "hw/qdev-core.h" > #include "qapi/error.h" > #include "qapi/qapi-commands-qdev.h" > @@ -240,13 +241,34 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, > return prop_list; > } > > -void qmp_object_add(const char *type, const char *id, > - bool has_props, QObject *props, Error **errp) > +void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp) > { > + QObject *props; > QDict *pdict; > Visitor *v; > Object *obj; > + const char *type; > + const char *id; > > + type = qdict_get_try_str(qdict, "qom-type"); > + if (!type) { > + error_setg(errp, QERR_MISSING_PARAMETER, "qom-type"); > + return; > + } else { > + type = g_strdup(type); > + qdict_del(qdict, "qom-type"); > + } > + > + id = qdict_get_try_str(qdict, "id"); > + if (!id) { > + error_setg(errp, QERR_MISSING_PARAMETER, "id"); > + return; > + } else { > + id = g_strdup(id); > + qdict_del(qdict, "id"); > + } > + > + props = qdict_get(qdict, "props"); > if (props) { > pdict = qobject_to(QDict, props); > if (!pdict) { > @@ -254,17 +276,23 @@ void qmp_object_add(const char *type, const char *id, > return; > } > qobject_ref(pdict); > - } else { > - pdict = qdict_new(); > + qdict_del(qdict, "props"); > + qdict_join(qdict, pdict, false); > + if (qdict_size(pdict) != 0) { > + error_setg(errp, "Option in 'props' conflicts with top level"); > + qobject_unref(pdict); > + return; > + } > + qobject_unref(pdict); > } > > - v = qobject_input_visitor_new(QOBJECT(pdict)); > - obj = user_creatable_add_type(type, id, pdict, v, errp); > + v = qobject_input_visitor_new(QOBJECT(qdict)); > + obj = user_creatable_add_type(type, id, qdict, v, errp); > visit_free(v); > if (obj) { > object_unref(obj); > } > - qobject_unref(pdict); > + *ret_data = QOBJECT(qdict_new()); > } > > void qmp_object_del(const char *id, Error **errp) >
Am 08.07.2020 um 17:48 hat Paolo Bonzini geschrieben: > On 06/03/20 18:14, Kevin Wolf wrote: > > Mapping object-add to the command line as is doesn't result in nice > > syntax because of the nesting introduced with 'props'. This becomes > > nicer and more consistent with device_add and netdev_add when we accept > > properties for the object on the top level instead. > > > > 'props' is still accepted after this patch, but marked as deprecated. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Message-Id: <20200224143008.13362-8-kwolf@redhat.com> > > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Hi Kevin and Markus, > > I just noticed this patch. In my opinion "nice syntax" is not a good > argument for having a "gen: false" command that is even less > introspectable than the "props" argument. Markus was going to introduce new QAPI schema syntax that would allow to specify a few options explicitly and then one option for "the rest" that would just be mapped to a QDict like "any" or "gen": false, and that wouldn't require any nesting. I'm not sure if any progress was made there, but making things consistent between device_add, netdev_add and object_add was a step in this direction anyway. > As an aside, it would have been nice to run this through Markus and me, > though in all fairness I'm not sure I would have been responsive back > in February. It went through my tree because of the other patches in the series, but I wrote this patch specifically at Markus's request. > I would like to un-deprecate this for 5.1, and revert it in either 5.1 > or 5.2. (Also I will be away next week, so the decision would have to > be taken quickly). Please discuss it with Markus then. Kevin
On 08/07/20 18:05, Kevin Wolf wrote: > Markus was going to introduce new QAPI schema syntax that would allow to > specify a few options explicitly and then one option for "the rest" that > would just be mapped to a QDict like "any" or "gen": false, and that > wouldn't require any nesting. Oh, I wasn't aware of that. That would be something like 'properties': 'remainder' I guess. That would be fine too. Paolo > I'm not sure if any progress was made there, but making things > consistent between device_add, netdev_add and object_add was a step in > this direction anyway. > >> As an aside, it would have been nice to run this through Markus and me, >> though in all fairness I'm not sure I would have been responsive back >> in February. > It went through my tree because of the other patches in the series, but > I wrote this patch specifically at Markus's request. > >> I would like to un-deprecate this for 5.1, and revert it in either 5.1 >> or 5.2. (Also I will be away next week, so the decision would have to >> be taken quickly). > Please discuss it with Markus then.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 08/07/20 18:05, Kevin Wolf wrote: >> Markus was going to introduce new QAPI schema syntax that would allow to >> specify a few options explicitly and then one option for "the rest" that >> would just be mapped to a QDict like "any" or "gen": false, and that >> wouldn't require any nesting. > > Oh, I wasn't aware of that. That would be something like 'properties': > 'remainder' I guess. That would be fine too. Glad I'm spared a design argument ;) >> I'm not sure if any progress was made there, but making things Not yet; error handling ate me whole, and spit me out only the day before yesterday or so. >> consistent between device_add, netdev_add and object_add was a step in >> this direction anyway. Yes. Permit me to digress. We ran into the "and also arbitray properties" need repeatedly, and tried several solutions over time. QAPI/QMP is big, and the left hand often wasn't too interested in what the right hand had been doing. If memory and my git archaeology skills serve, the first instance was device_add: 3418bd25e1 "qdev hotplug: infrastructure and monitor commands." 2009-10-05 Simple adaption of the QemuOpts-based -device for HMP. Since QemuOpts is flat, so is device_add. QAPI didn't exist, yet. A QMP version followed: 8bc27249f0 "monitor: convert do_device_add() to QObject" 2010-03-16 Just as flat. Next was netdev_add: ae82d3242d "monitor: New commands netdev_add, netdev_del" 2010-04-18 Flat for the same reason. When QAPI came along in ebffe2afce "Merge remote-tracking branch 'qmp/queue/qmp' into staging" 2011-10-10 these two commands were left unQAPIfied, as the schema language could not express "and also arbitrary properties". Soon "solved" with a cop out: 5dbee474f3 "qapi: allow a 'gen' key to suppress code generation" 2011-12-15 A half-hearted QAPIfication of netdev_add followed: 928059a37b "qapi: convert netdev_add" 2012-06-04 QAPI schema: { 'command': 'netdev_add', 'data': {'type': 'str', 'id': 'str', '*props': '**'}, 'gen': 'no' } Note the bogus type '**', which only works because 'gen': 'no' also bypasses type checking (which you wouldn't guess from the commit message, documentation, or even the schema). In fact, the whole 'props' thing is a lie: there is no such parameter, the command is as flat as it ever was. Fixed in b8a98326d5 "qapi-schema: Fix up misleading specification of netdev_add" 2015-09-21 { 'command': 'netdev_add', 'data': {'type': 'str', 'id': 'str'}, 'gen': false } but by then it had already spread to object-add, with an equally bogus type 'dict': cff8b2c6fc "monitor: add object-add (QMP) and object_add (HMP) command" 2014-01-06 { 'command': 'object-add', 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'}, 'gen': 'no' } Only 'props' was real this time: you really had to wrap the properties in "props": { ... }. Non-flat. Meh. Eventually, even device_add made it into the schema: 94cfd07f26 "qapi-schema: add 'device_add'" 2016-09-19 { 'command': 'device_add', 'data': {'driver': 'str', 'id': 'str'}, 'gen': false } # so we can get the additional arguments And netdev_add was finally done right: db2a380c84 "net: Complete qapi-fication of netdev_add" 2020-03-17 { 'command': 'netdev_add', 'data': 'Netdev', 'boxed': true } Doing device_add and object-add right is harder (impractical?), because their schema would be a union with one branch per device / object type. End of digression. >>> As an aside, it would have been nice to run this through Markus and me, >>> though in all fairness I'm not sure I would have been responsive back >>> in February. >> It went through my tree because of the other patches in the series, but >> I wrote this patch specifically at Markus's request. Yes. We discussed how to best satisfy Kevin's immediate needs without making other problems harder. Perhaps we should have posted a summary to the list. >>> I would like to un-deprecate this for 5.1, and revert it in either 5.1 >>> or 5.2. (Also I will be away next week, so the decision would have to >>> be taken quickly). >> Please discuss it with Markus then.
diff --git a/qapi/qom.json b/qapi/qom.json index ecc60c4401..8abe998962 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -210,7 +210,12 @@ # # @id: the name of the new object # -# @props: a dictionary of properties to be passed to the backend +# @props: a dictionary of properties to be passed to the backend. Deprecated +# since 5.0, specify the properties on the top level instead. It is an +# error to specify the same option both on the top level and in @props. +# +# 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 @@ -221,12 +226,13 @@ # # -> { "execute": "object-add", # "arguments": { "qom-type": "rng-random", "id": "rng1", -# "props": { "filename": "/dev/hwrng" } } } +# "filename": "/dev/hwrng" } } # <- { "return": {} } # ## { 'command': 'object-add', - 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} } + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'}, + 'gen': false } # so we can get the additional arguments ## # @object-del: diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 1eaa559079..6c1d9034d9 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -190,6 +190,11 @@ Use ``migrate-set-parameters`` instead. Use ``migrate-set-parameters`` and ``query-migrate-parameters`` instead. +``object-add`` option ``props`` (since 5.0) +''''''''''''''''''''''''''''''''''''''''''' + +Specify the properties for the object as top-level arguments instead. + ``query-block`` result field ``dirty-bitmaps[i].status`` (since 4.0) '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h index 3e4e1d928b..6f92f3cebb 100644 --- a/include/qom/object_interfaces.h +++ b/include/qom/object_interfaces.h @@ -162,4 +162,11 @@ void 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 686bbc3f0d..3885464513 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -18,6 +18,7 @@ #include "qapi/visitor.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" +#include "qom/object_interfaces.h" #include "hw/xen/xen_common.h" #include "hw/block/xen_blkif.h" #include "hw/qdev-properties.h" @@ -858,10 +859,18 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id, { XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1); Error *local_err = NULL; + QDict *opts; + QObject *ret_data; iothread->id = g_strdup(id); - qmp_object_add(TYPE_IOTHREAD, id, false, NULL, &local_err); + opts = qdict_new(); + qdict_put_str(opts, "qom-type", TYPE_IOTHREAD); + qdict_put_str(opts, "id", id); + qmp_object_add(opts, &ret_data, &local_err); + qobject_unref(opts); + qobject_unref(ret_data); + if (local_err) { error_propagate(errp, local_err); diff --git a/monitor/misc.c b/monitor/misc.c index 6c41293102..1748ab3911 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -248,6 +248,8 @@ static void monitor_init_qmp_commands(void) QCO_NO_OPTIONS); qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_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 6136efec16..49db926fcc 100644 --- a/qom/qom-qmp-cmds.c +++ b/qom/qom-qmp-cmds.c @@ -14,6 +14,7 @@ */ #include "qemu/osdep.h" +#include "block/qdict.h" #include "hw/qdev-core.h" #include "qapi/error.h" #include "qapi/qapi-commands-qdev.h" @@ -240,13 +241,34 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, return prop_list; } -void qmp_object_add(const char *type, const char *id, - bool has_props, QObject *props, Error **errp) +void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp) { + QObject *props; QDict *pdict; Visitor *v; Object *obj; + const char *type; + const char *id; + type = qdict_get_try_str(qdict, "qom-type"); + if (!type) { + error_setg(errp, QERR_MISSING_PARAMETER, "qom-type"); + return; + } else { + type = g_strdup(type); + qdict_del(qdict, "qom-type"); + } + + id = qdict_get_try_str(qdict, "id"); + if (!id) { + error_setg(errp, QERR_MISSING_PARAMETER, "id"); + return; + } else { + id = g_strdup(id); + qdict_del(qdict, "id"); + } + + props = qdict_get(qdict, "props"); if (props) { pdict = qobject_to(QDict, props); if (!pdict) { @@ -254,17 +276,23 @@ void qmp_object_add(const char *type, const char *id, return; } qobject_ref(pdict); - } else { - pdict = qdict_new(); + qdict_del(qdict, "props"); + qdict_join(qdict, pdict, false); + if (qdict_size(pdict) != 0) { + error_setg(errp, "Option in 'props' conflicts with top level"); + qobject_unref(pdict); + return; + } + qobject_unref(pdict); } - v = qobject_input_visitor_new(QOBJECT(pdict)); - obj = user_creatable_add_type(type, id, pdict, v, errp); + v = qobject_input_visitor_new(QOBJECT(qdict)); + obj = user_creatable_add_type(type, id, qdict, v, errp); visit_free(v); if (obj) { object_unref(obj); } - qobject_unref(pdict); + *ret_data = QOBJECT(qdict_new()); } void qmp_object_del(const char *id, Error **errp)