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

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

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

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

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

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

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

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.

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

> +    /* private */

You should say "private for libxl__qmp_cmd_...".

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

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

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

I hope this review is helpful.

Thanks,
Ian.

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