Re: [Qemu-devel] [RFC v5 11/26] qmp: introduce QMPCapability

[Fam Zheng <famz@redhat.com>]

On Fri, 12/15 17:14, Peter Xu wrote:
> On Wed, Dec 13, 2017 at 04:56:51PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Dec 05, 2017 at 01:51:45PM +0800, Peter Xu wrote:
> > > There was no QMP capabilities defined.  Define the first "oob" as
> > > capability to allow out-of-band messages.
> > > 
> > > Also, touch up qmp-test.c to test the new bits.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  monitor.c        | 15 +++++++++++++--
> > >  qapi-schema.json | 13 +++++++++++++
> > >  tests/qmp-test.c | 10 +++++++++-
> > >  3 files changed, 35 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/monitor.c b/monitor.c
> > > index e8f5a586e4..bad6ee8dd1 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -3944,12 +3944,23 @@ void monitor_resume(Monitor *mon)
> > >  
> > >  static QObject *get_qmp_greeting(void)
> > >  {
> > > +    QDict *result = qdict_new(), *qmp = qdict_new();
> > > +    QList *cap_list = qlist_new();
> > >      QObject *ver = NULL;
> > > +    QMPCapability cap;
> > > +
> > > +    qdict_put(result, "QMP", qmp);
> > >  
> > >      qmp_marshal_query_version(NULL, &ver, NULL);
> > > +    qdict_put_obj(qmp, "version", ver);
> > > +
> > > +    for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> > > +        qlist_append(cap_list, qstring_from_str(
> > > +                         QMPCapability_str(cap)));
> > > +    }
> > > +    qdict_put(qmp, "capabilities", cap_list);
> > >  
> > > -    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
> > > -                              ver);
> > > +    return QOBJECT(result);
> > >  }
> > 
> > Why did you replace qobject_from_jsonf() with manual qdict_*() calls?
> > 
> > I was expecting this (it's shorter and easier to read):
> > 
> >   static QObject *get_qmp_greeting(void)
> >   {
> >       QList *cap_list = qlist_new();
> >       QObject *ver = NULL;
> >       QMPCapability cap;
> > 
> >       qmp_marshal_query_version(NULL, &ver, NULL);
> > 
> >       for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> >           qlist_append(cap_list, qstring_from_str(
> >                            QMPCapability_str(cap)));

And aligning the parameters would be even nicer.

> >       }
> > 
> >       return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}",
> >                                 ver, cap);
> 
> (I believe you mean s/cap/cap_list/ here?)
> 
> >   }
> 
> Oh I just didn't notice that "%p" magic at all... :(
> 
> I think for me it's fine in either way.  Frankly speaking creating the
> objects explicitly would be even easier to understand for me instead
> of using a mixture of two ways... But just let me know if you want me
> to do it your way.  I can switch.  Thanks,

I agree with Stefan here. (Readability is not judged based on how low level the
code goes when there is a higher level interface available, it's exactly the
opposite, and this doesn't change even when you happen to not know it.)

Fam