Re: [PATCH v6.2 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*

[Ian Jackson <ian.jackson@citrix.com>]

Anthony PERARD writes ("Re: [PATCH v6.2 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> On Fri, Nov 16, 2018 at 04:09:03PM +0000, Ian Jackson wrote:
> > Anthony PERARD writes ("Re: [PATCH v6.2 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> > > I wanted to reduce the number of function calls between when a user
> > > callback is called and when ev_qmp's control passes outsite of ev_qmp's
> > > implementation. So I moved qmp_ev_handle_message() call here.
> > 
> > Why did you want to reduce the number of function calls ?
> 
> Well, there is a need to track that `ev` might be discarded, and the
> only way left is via a return value. It is probably easier to follow if
> less function have the possibility that a user callback have been
> called.

Oh, I see.  Hrm.  OK, I think I would have done it the other way, but
that makes sense.

> > I think a bare \n is not legal and should be treated as a protocol
> > error.  Don't you agree ?  Given that, you can search for \n, and if
> > it is not preceded by \r, call it an error.
> 
> I can't find anything that say bare \n are not legal. \n is part of
> rfc7159, which the qmp-spec mentions when speaking about JSON data
> structures. It is even possible to ask QEMU to add bare \n, this is done
> with pretty=on.

I think I was wrong.  So thanks for explaining and you should leave
this code the way it is.

> > I'm not sure I can see sensible a way of doing this that doesn't have
> > *three* id variables:
> >   - a counter for generating new ids
> >   - the id put in the capabilities command
> >   - the id put in the user's command
> 
> That sounds fine.

If you have another approach to suggest please go ahead.

> > Oh, is the purpose to inform qemu what our capabilities are ?
> 
> It actually inform qemu of the capabilities we want. The QMP server will
> not accept any other commands before the client execute qmp_capabilities.
> 
> The conversation between QEMU and libxl goes like this:
> 
> QEMU: Hi, I'm QEMU 3.0, I can do "oob".
> libxl: Hi, I don't need any capabilities.
> QEMU: Thanks, you can now run any other command you like.
> libxl sends the user command.
> 
> Or in term of edited JSON:
> QEMU:  { "QMP": {"version": XXX, "capabilities": ["oob"] } }
> libxl: { "execute": "qmp_capabilities", "arguments": {} }
> QEMU:  { "return": {}}

Thanks for the explanation.

> And here is a link to the example in the QMP spec document
> (3. QMP Examples):
> https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/qmp-spec.txt;h=8f7da0245d51447be7df2b3d4b105bad9fbec0b3;hb=HEAD#l244
> 
> Maybe I should add somewhere in a comment where to find the QMP spec,
> even so we already have "This file implement a client for QMP (QEMU
> Monitor Protocol). For the Specification, see in the QEMU repository."
> at the top of libxl_qmp.c

Sorry for pontificating without having read the spec myself ...  but
yes a link would be nice.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel