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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jun 27, 2018 at 04:07:52PM +0100, Ian Jackson wrote:
> 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.

That sound plausible, I can try to move this state to libxl__ao. I just
need to find out what's happen on connect(3) when something else is
allready connected to QEMU.

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

That is what I have done. The libxl__ev_qmp_start() should be static, it
is only use from within libxl_qmp.c. But I've put it in libxl_interal.h
just to allow this patch to build.

The _stop is normally not needed, to be called. It is automatically
called as soon as there is no more QMP command in flight (all command
have receveid a response).

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

So what the interface looks like at the end of the series is:

void libxl__ev_qmp_init(libxl__ev_qmp *ev);
int libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev,
                           libxl__ev_qmp_callback *,
                           uint32_t domid,
                           const char *cmd, libxl__json_object *args);
void libxl__ev_qmp_deregister(libxl__gc *gc, libxl__ev_qmp *ev);
int libxl__ev_qmp_isregistered(const libxl__ev_qmp *ev);

The libxl__qmp_state is not exposed. The _register() will attempt to
find the current _state and use it, or create a new one (connect).

The libxl__ev_qmp_stop() call in libxl_ctx_free() is only there in case
something went wrong, it should not be needed.


I'll try to write better documentation about the possible states of both
libxl__ev_qmp_state and libxl__ev_qmp, and how they relate to each
other.

BTW, I think qmp is kind of similair to libxl__ev_xswatch, which would
_ev_fd_register(xenstore_fd) on the first path to watch, and _deregister
once there is no more watches. The one more thing that qmp need to do is
open the socket and close it.

-- 
Anthony PERARD

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