[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v5 11/26] qmp: introduce QMPCapability
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [RFC v5 11/26] qmp: introduce QMPCapability |
Date: |
Sat, 16 Dec 2017 11:58:40 +0800 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Fri, Dec 15, 2017 at 05:38:03PM +0800, Fam Zheng wrote:
> 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 <address@hidden>
> > > > ---
> > > > 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.)
I was talking mostly about using two ways or using only one way to do
this (especially that's quite simple even using raw objects...),
rather than which one I know. :)
Again, since I'm happy with either way actually, I'm giving in. Thanks,
--
Peter Xu
- Re: [Qemu-devel] [RFC v5 09/26] monitor: create monitor dedicate iothread, (continued)
[Qemu-devel] [RFC v5 10/26] monitor: allow to use IO thread for parsing, Peter Xu, 2017/12/05
[Qemu-devel] [RFC v5 11/26] qmp: introduce QMPCapability, Peter Xu, 2017/12/05
[Qemu-devel] [RFC v5 12/26] qmp: negociate QMP capabilities, Peter Xu, 2017/12/05
[Qemu-devel] [RFC v5 13/26] qmp: introduce some capability helpers, Peter Xu, 2017/12/05