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

[Paolo Bonzini]

On 15/03/21 12:38, Dr. David Alan Gilbert wrote:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> > > Paolo Bonzini <pbonzini@redhat.com> writes:
> > >
> > > > On 13/03/21 14:28, Markus Armbruster wrote:
> > > > > Kevin Wolf <kwolf@redhat.com> 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@redhat.com>
> > > > > > Acked-by: Peter Krempa <pkrempa@redhat.com>
> > > > > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > > ---
> > > > > >    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.

I think we should try to improve the string-value QObject visitor to 
also allow JSON values in some places, for example to allow

object_add 
memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=[0,1,2,3]

Paolo