Re: [Xen-devel] [PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Jul 03, 2018 at 03:48:22PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3.1] libxl: Design of an async API to issue 
> QMP commands to QEMU"):
> > All the functions will be implemented in later patches.
> 
> Thanks, this really makes things clearer for me.
> 
> > What do you think of this design? This is the same as in my patch series
> > with new names (to avoid confusion with libxl___ev_*) and documentation.
> > 
> > I'll write something as well for the internal of the engine (the QMP
> > client itself).

Before replying to this email, here is the description of what state a
QMP connection can be. This states are only internal to libxl_qmp.c, the
rest of libxl doesn't need to know.

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1235,6 +1235,97 @@
     return ret;
 }
 
+/* ------------ Implementation of asynchronous QMP calls ------------ */
+
+/*
+ * This are different state possible when the QMP client (libxl) is waiting.
+ * Transition from one state to the other is listed after.
+ *
+ * States:
+ *   Undefined
+ *   Disconnected
+ *      nothing, no allocated ressources
+ *   Connecting
+ *      Have allocated ressources,
+ *      Have connected to the QMP socket,
+ *      Waiting for server greeting.
+ *   Capability Negociation
+ *      QMP server is in Capabilities Negotiation mode.
+ *      Waiting for a response to the "qmp_capabilities" command
+ *   Connected
+ *      QMP server is in command mode,
+ *      commands can be issued
+ *
+ * Here is the transition from one state to the next:
+ *  Disconnected -> Connecting:
+ *    Connect to the QMP socket.
+ *  Connecting -> Capability Negociation
+ *    Server greeting received
+ *    Send "qmp_capabilities"
+ *  Capability Negociation -> Connected
+ *    Response to "qmp_capabilities" received"
+ *
+ * checkout qemu.git:docs/interop/qmp-spec.txt for more information.
+ */


> > +/*
> > + * This struct is used to register one command to send to QEMU with an
> > + * associated callback.
> 
> You still use the word `register' which I don't think is right.  It
> makes it sound as if there is a separate `registration' and `sending'.
> 
> How about
> 
>    This facility allows a command to be sent to qemu, and the response
>    to be handed to a callback function.  Each libxl__qmp_cmd_state
>    handles zero or one outstanding command; if multiple commands are
>    to be sent concurrently, multiple libxl__qmp_cmd_state's must be
>    used.
> 
> or some such ?

That's looks better.

> > + * Possible states:
> > + *  Undefined
> > + *    Might contain anything.
> > + *  Idle
> > + *    Struct contents are defined enough to pass to any
> > + *    libxl__qmp_cmd_* functions but is not registered and callback
> > + *    will not be called. The struct does not contain references to
> > + *    any allocated resources so can be thrown away.
> > + *  Active
> > + *    Currently waiting for a response from QEMU, and callback can be
> > + *    called. _dispose must be called to reclaim resources.
> 
> I don't think this set of states is accurate.  In particular, your API
> description (about connection management) implies that there are at
> least these states:
>   (i) undefined
>   (ii) there is no qmp connection open
>   (iii) we have sent a command and are expecting a callback
>   (iv) there is a qmp connection open, but no command is outstanding
> 
> (iv) does not fit into any of the categories above.

I don't think the state of a qmp connection can fit into
libxl__qmp_cmd_state. The "Active" state doesn't mean that a qmp
connection is open or that the command have been sent. It merely mean
that the syscall socket(3) and connect(3) have return successfully, and
there will be an attempt later to sent the command to qemu.

> > +/*
> > + * Initialize libxl__qmp_cmd_state.
> > + *    Which must be in Undefined or Idle state.
> > + *    On return it is Idle.
> 
> You might want to abbreviate this state notation, eg as is done for
> libxl__xs_transaction_... .  So here you could just write
>        Undefined/Idle -> Idle

Will do.

> > +/*
> > + * Register a command to be issued to QEMU.
> 
> Again, "register" has been inherited from libxl_ev_*.  I think it
> would be clearer to say that this function
> 
>      Sends a command to QEMU.

The API I'm describing haven't send anything to QEMU yet, the command is
merely put in a queue, and an attempt to send it will be done later,
when QEMU is ready to receive commands and when the socket is not
blocked.

> Looking through libxl_internal.h again, I see that there is
> libxl__ev_child, which provides another model for handling a thing
> which is sort of, but not exactly, like a libxl__ev_FOO.  There, the
> struct is called libxl_ev_*, but the actual function names are quite
> different.  There is no libxl__ev_child_register, only
> libxl__ev_child_fork.  And libxl__ev_child_deregister is entirely
> absent.
> 
> So you could call your think libxl__ev_qmp but have functions
> libxl__ev_qmp_send and libxl__ev_qmp_dispose/destroy.
> (and _init of course).
> 
> If you do this you need to do like libxl__ev_child_fork does, and
> write commentary describing the ways in which a libxl__ev_qmp is not
> like most libxl__ev_FOO.
> 
> I think I mind less what the struct is called than what the functions
> are called.  Your send function should probably be _send or _exec.
> The dispose function can be _dispose or _destroy, or similar.

I look into that.

> > +struct libxl__qmp_cmd_state {
> > +    /* read-only once Active and from within the callback */
> > +    uint32_t domid;
> > +    libxl__qmp_cmd_callback *callback;
> 
> You copied this pattern from libxl__ev_FOO.  I don't object to it.
> 
> But in general, I have sometimes found it more convenient to put these
> parameters in the struct and expect callers to fill them in.  You are
> doing that with the carefd.  Maybe you want to do it with all of
> them ?  See libxl__datacopier_start for an example.
> 
> Up to you.  I don't object to mixing the two styles within the same
> facility provided the internal docs are clear.

I've done the carefd differently because I didn't know how to put it
into the function parameters, beside having a new function. But your
sugestion about having the caller fill the struct sound good. I'll do
that. That way, a caller will know in which states are the different
fields.

> > +    /* private */
> 
> You should say "private for libxl__qmp_cmd_...".

Will do.

> > +    /*
> > +     * This value can be initialise before calling _qmp_cmd_exec. The
> > +     * file descriptor will sent to QEMU along with the command, then
> > +     * the fd will be closed.
> > +     */
> > +    libxl__carefd *efd;
> 
> Why not
>        libxl__carefd efd;
> ?

The libxl__carefd_* API returns a newly allocated efd, a pointer. I
can't preallocate it.

> Also, I don't think this description of the semantics is right.  The
> caller must always somehow arrange to initialise this value.
> Presumably _init clears it ?  Certainly this as a parameter to the
> operation, this should be up with domid and callback.
> 
> Maybe you want comments like the ones in libxl__datacopier_state etc.,
> which say /* caller must fill these in */.
> 
> And, you probably want to make it clear that the fd remains open in
> the libxl process.  (I assume it does.)

Well I was closing the fd once it was sent to QEMU. But we can have the
callbacks takes care of closing it instead.

> > +libxl__qmp_error_class = Enumeration("qmp_error_class", [
> > +    # No error
> > +    (0, "NONE"),
> > +    # Error generated by libxl (e.g. socket closed unexpectedly, no mem, 
> > ...)
> > +    (1, "libxl_error"),
> > +    # QMP error classes described in QEMU sources code (QapiErrorClass)
> > +    (2, "GenericError"),
> > +    (3, "CommandNotFound"),
> > +    (4, "DeviceNotActive"),
> > +    (5, "DeviceNotFound"),
> > +    # Unrecognized QMP error class
> > +    (6, "Unknown"),
> 
> Are these numbers from qmp ?  Why not assign a bunch of libxl error
> values instead ?

No, these are strings from QEMU, numbers doesn't matter. Also I don't
know how well those are going to fit into libxl errors.

FYI, here is the description of the different errors generated by QEMU:

(qemu.git:qapi/common.json)
# @QapiErrorClass:
#
# QEMU error classes
#
# @GenericError: this is used for errors that don't require a specific error
#                class. This should be the default case for most errors
#
# @CommandNotFound: the requested command has not been found
#
# @DeviceNotActive: a device has failed to be become active
#
# @DeviceNotFound: the requested device has not been found

QEMU always associate an error messages when it send an error, so the
only thing t odo with GenericError is to log that message.

> I hope this review is helpful.
> 
> Thanks,
> Ian.

-- 
Anthony PERARD

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