Re: [PATCH v3 27/30] hmp: QAPIfy object_add

* Kevin Wolf (kwolf(a)redhat.com) wrote: > Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben: >> Paolo Bonzini <pbonzini(a)redhat.com&gt; writes: >> >>> On 13/03/21 14:28, Markus Armbruster wrote: >>>> Kevin Wolf <kwolf(a)redhat.com&gt; writes: >>>> >>>>> This switches the HMP command object_add from a QemuOpts-based parser to >>>>> user_creatable_add_from_str() which uses a keyval parser and enforces >>>>> the QAPI schema. >>>>> >>>>> Apart from being a cleanup, this makes non-scalar properties and help >>>>> accessible. In order for help to be printed to the monitor instead of >>>>> stdout, the printf() calls in the help functions are changed to >>>>> qemu_printf(). >>>>> >>>>> Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; >>>>> Acked-by: Peter Krempa <pkrempa(a)redhat.com&gt; >>>>> Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; >>>>> Reviewed-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com&gt; >>>>> --- >>>>> monitor/hmp-cmds.c | 17 ++--------------- >>>>> qom/object_interfaces.c | 11 ++++++----- >>>>> hmp-commands.hx | 2 +- >>>>> 3 files changed, 9 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >>>>> index 3c88a4faef..652cf9ff21 100644 >>>>> --- a/monitor/hmp-cmds.c >>>>> +++ b/monitor/hmp-cmds.c >>>>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) >>>>> >>>>> void hmp_object_add(Monitor *mon, const QDict *qdict) >>>>> { >>>>> + const char *options = qdict_get_str(qdict, "object"); >>>>> Error *err = NULL; >>>>> - QemuOpts *opts; >>>>> - Object *obj = NULL; >>>>> - >>>>> - opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err); >>>>> - if (err) { >>>>> - goto end; >>>>> - } >>>>> >>>>> - obj = user_creatable_add_opts(opts, &err); >>>>> - qemu_opts_del(opts); >>>>> - >>>>> -end: >>>>> + user_creatable_add_from_str(options, &err); >>>>> hmp_handle_error(mon, err); >>>>> - >>>>> - if (obj) { >>>>> - object_unref(obj); >>>>> - } >>>>> } >>>> >>>> Doesn't this break the list-valued properties (Memdev member host-nodes, >>>> NumaNodeOptions member cpus) exactly the same way that made us keep >>>> QemuOpts for qemu-system-FOO -object? >>> >>> Yes, it does. I guess it can just be documented, unlike for the command >>> line? >> >> Maybe. Judgement call, not mine to make. >> >> Do people create such objects in HMP? I figure we don't really know. >> Educated guess? >> >> If you try, how does it break? Is it confusing? Can you show an >> example? > > (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0 > Error: Invalid parameter type for 'host-nodes', expected: array > (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0 > (qemu) > > HMP is not a stable interface, so changing the syntax didn't feel like a > problem to me. I doubt many people do HMP memory hotplug while setting a > specific NUMA policy, but it wouldn't change my assessment anyway. I > should have made this explicit in the commit message, though. I'm OK for it to change, but yes I'd like to have the before/after syntax listed somewhere as easy references for people confused.