Re: [Xen-devel] [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v3 14/31] libxl_qmp_ev: Introduce 
libxl__ev_qmp_start() to connect to QMP"):
> This is a first patch to implement libxl__ev_qmp, it only connect to the
> QMP socket of QEMU and register a callback that does nothing.
...
> @@ -503,6 +504,9 @@ struct libxl__ctx {
>      LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
>  
>      libxl_version_info version_info;
> +
> +    // FIXME: May need a list, with on state for each domid
> +    libxl__ev_qmp_state *qmp_ev;

My thought is that the lifetime of this thing should probably be in
each relevant ao.

Is it too inconvenient to reconnect to qmp for every libxl operation ?
If it is then this needs to be a cache, a bit like libxl__poller but
different.  But that can be handled inside what you are calling
_ev_qmp_start.

Also, I think if you are going to have a libxl__ev_qmp it needs to be
just like all the other libxl__ev_ things.  It's not clear to me that
QMP is similar enough.

Do you actually need an explicit "start" or "connect" operation ?
I think in any case the "send a qmp command" operations should
probably connect automatically.

So something like this:

   /* libxl__qmp_state has the following states:
    *   Undefined
    *   Disconnected
    *   Connected
    */

   void libxl__qmp_init(ibxl__qmp_state *qmp); /* U -> D */

   int libxl__qmp_connect(libxl__gc *gc, uint32_t domid,
                          libxl__qmp_state *qmp_upd); /* [UC] -> C */

   int libxl__qmp_dispose(libxl__qmp_state *qmp_upd); /* [DC] -> D */

   int libxl__qmp_command( lots of parameters incl callback ); /* [DC] */

> +_hidden libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t 
> domid);

Line length.

Also, this interface does not support returning a proper error status.

> +libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t domid)
> +{
> +    int rc, r;
> +    struct sockaddr_un un;
> +    const char *qmp_socket_path;
> +    libxl__ev_qmp_state *qmp;
...
> +out:
> +    if (rc)
> +        libxl__ev_qmp_stop(gc, qmp);
> +    CTX_UNLOCK;
> +    return qmp;

I think the error handling is messed up here.  If this fails, you will
stop the qmp and then return it anyway.

Ian.

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