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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Jul 27, 2018 at 03:05:54PM +0100, Anthony PERARD wrote:
> All the functions will be implemented in later patches.
> 
> This patch includes the API that libxl can use to send QMP commands to
> QEMU.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_internal.h | 76 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 5b71a23d23..c453ac10a5 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -202,6 +202,8 @@ typedef struct libxl__ao libxl__ao;
>  typedef struct libxl__aop_occurred libxl__aop_occurred;
>  typedef struct libxl__osevent_hook_nexus libxl__osevent_hook_nexus;
>  typedef struct libxl__osevent_hook_nexi libxl__osevent_hook_nexi;
> +typedef struct libxl__json_object libxl__json_object;
> +typedef struct libxl__carefd libxl__carefd;
>  
>  typedef struct libxl__domain_create_state libxl__domain_create_state;
>  typedef void libxl__domain_create_cb(struct libxl__egc *egc,
> @@ -357,6 +359,76 @@ struct libxl__ev_child {
>      LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
>  };
>  
> +/*
> + * QMP asynchronous calls
> + */

Place the title in the same comment block?

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

> + * 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
> + *
> + * Only one connection at a time can be made to one QEMU, so avoid keeping a
                                                               ^ to
> + * libxl__ev_qmp Connected for to long and call libxl__ev_qmp_dispose as soon
                    ^ unneeded cap        ^ remove 'and'
> + * 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.'

> + *  Active
> + *    Currently waiting for the callback to be called.
> + *    _dispose must be called to reclaim resources.
> + *  Connected
> + *    Struct contain allocated ressources.
> + *    Calling _send() with this same ev will use the same QMP connection.
> + *    _dispose() must be called to reclaim resources.
> + *
> + * libxl__ev_qmp_init: Undefined/Idle -> Idle
> + *
> + * libxl__ev_qmp_send: Idle/Connected -> Active (on error: Idle)
> + *    Sends a command to QEMU.
> + *    callback will be called when a response is received or when an error
> + *    as occured.
> + *
> + * libxl__ev_qmp_dispose: Connected/Active/Idle -> Idle
> + *
> + * callback: When called: Active -> Connected
> + *    When called, ev is Connected and can be reused or disposed of.
> + *    When an error occured, it is called with response == NULL and the error
         ^ If
> + *    code in rc.
> + *    The callback is only called once.
> + */
> +typedef struct libxl__ev_qmp libxl__ev_qmp;
> +typedef void libxl__ev_qmp_callback(libxl__egc *egc, libxl__ev_qmp *ev,
> +                                    const libxl__json_object *response,
> +                                    int rc);
> +
> +_hidden void libxl__ev_qmp_init(libxl__ev_qmp *ev);
> +_hidden int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> +                               const char *cmd, libxl__json_object *args);
> +_hidden void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev);
> +
> +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.

> +    libxl__ev_qmp_callback *callback;
> +    libxl__carefd *cfd; /* set to send a fd with the command, NULL otherwise */
> +
> +    /* remaining fields are private to libxl_ev_qmp_* */
> +

Extra newline?

> +    int id;
> +};
> +

Thanks, Roger.

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