Re: [Xen-devel] [PATCH v4 12/32] libxl: Design of an async API to issue QMP commands to QEMUç

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Aug 03, 2018 at 03:55:48PM +0200, Roger Pau Monné wrote:
> On Fri, Aug 03, 2018 at 12:18:02PM +0100, Anthony PERARD wrote:
> > On Thu, Aug 02, 2018 at 11:01:43AM +0200, Roger Pau Monné wrote:
> > > On Fri, Jul 27, 2018 at 03:05:54PM +0100, Anthony PERARD wrote:
> > > > +/*
> > > > + * This facility allows a command to be sent to QEMU, and the response 
> > > > to be
> > > > + * handed to a callback function.  Each libxl__ev_qmp handles zero or 
> > > > one
> > > 
> > > Do you really mean 'zero or one' or 'zero or more'?
> > 
> > It is zero or one. But I'm not sur that extra sentence is usefull.
> > Initially, it was followed by "if multiple commands are to be sent
> > concurrently, multiple libxl__ev_qmp's must be used."
> > 
> > But concurency between multiple libxl__ev_qmp isn't possible, the second
> > libxl__ev_qmp created will have to wait until the first one is been
> > disposed of. But sending multiple command can be done, by waiting for
> > the first one to be done, then then the second one. This is why I have
> > the paragraph about "commands can be chained".
> 
> Then maybe you want to add something along the lines:
> 
> "Multiple QMP commands can be queued in a single libxl__ev_qmp
> instance, however they will be dispatched in a serialized fashion.
> Note also that there can only be a single libxl__ev_qmp instance
> active at a time."

That's not true, I don't think adding facilities to queue commands will
be usefull, usually, one will need to wait for a response to the first
command in order to prepare a second one. It is rare in libxl that
several commands can be prepared and queued in advance. User of this API
can do the queuing themself.

As for the single active instance of libxl__ev_qmp, that isn't true
easier, several libxl__ev_qmp can be in the state "Active" at the same
time. But only one can be in the "Connected" state at the same time (in
relation to a single qemu) due to the way qemu handles QMP connections.

> > > > + * outstanding command.
> > > > + *
> > > > + * Commands can be chained, with a same connection. (e.g. "add-fd" 
> > > > will need to
> > >                                     ^ the
> > > > + * be chained to the next command). A libxl__ev_qmp can be reused when 
> > > > the
> > >                                                                      ^ 
> > > after
> > > > + * callback is been called in order to use the same connection.
> > >                ^ has
> > 
> > Writing "after the callback has been called", would that mean that one
> > would need to wait that the callback returns before reusing the
> > libxl__ev_qmp?
> > 
> > That is what I meant, there is no need to wait for the callback to
> > return before reusing the same libxl__ev_qmp.
> 
> You mean that the same ev_qmp can be reused from the callback itself
> in order to send more QMP requests?

Yes, that's right.

> > > > + *
> > > > + * Only one connection at a time can be made to one QEMU, so avoid 
> > > > keeping a
> > >                                                                ^ to
> > 
> > "To avoid" feels like a how-to, I intended to write what should be done
> > and that it is not an option.
> > 
> > > > + * libxl__ev_qmp Connected for to long and call libxl__ev_qmp_dispose 
> > > > as soon
> > >                     ^ unneeded cap        ^ remove 'and'
> > 
> > The use of the capital was an attempt to say that it is one of the
> > possible states in which libxl__ev_qmp can be.
> 
> Yes, I realized about that later.

:-), good.

> > > > +struct libxl__ev_qmp {
> > > > +    /* caller should include this in their own struct */
> > > > +    /* caller must fill these in, and they must all remain valid */
> > >                                                    ^ no need for 'all'
> > > > +    uint32_t domid;
> > > 
> > > Strictly speaking domid is an uint16_t.
> > 
> > Nothing in libxl agrees with this statement. domid is almost always
> > stored in an uint32_t (sometime an int). There's event a libxl_domid
> > type (I just found out!) which is uint32_t.
> > 
> > Maybe I could try to use libxl_domid, and try to spread it's usage
> > inside libxl.
> 
> Yes, that would be my preference. I didn't know either that there was
> a libxl_domid type.

Well, beside the generated headers (libxl_types.idl), there is only 4
use of it (or 2 functions prototypes).

Thanks,

-- 
Anthony PERARD

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