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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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."

> > > + * 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?

> > > + *
> > > + * 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.

> > > + * as it is not needed anymore.
> > > + *
> > > + * Possible states of a libxl__ev_qmp:
> > > + *  Undefined
> > > + *    Might contain anything.
> > > + *  Idle
> > > + *    Struct contents are defined enough to pass to any libxl__ev_qmp_*
> > > + *    functions.
> >          ^ function
> > > + *    The struct does not contain references to any allocated private 
> > > resources
> > > + *    so can be thrown away.
> > 
> > I would add '... can be thrown away without any teardown.'
> 
> I don't think that's usefull to point out. One would have to call the
> teardown function (_dispose) in order to have an Idle state, anyway.

OK.

> > > +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.

Roger.

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