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

[Anthony PERARD <anthony.perard@citrix.com>]

On Thu, Jun 28, 2018 at 12:44:28PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP"):
> > 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).
> 
> Ah, I see.
> 
> Err, the global (ctx) state is definitely wrong then.  Multiple
> operations might be in flight at once with the same ctx[1], so libxl
> might need to be connected to multiple qemus (or connected to the same
> qemu multiple times, if you don't want to deal with demultiplexing).

Demultiplexing is already been dealt with, even in the current code, so
that's not an issue.

I haven't dealt with the ability to connect to multiple qemu (of
different domain) but I know that was an issue, there a TODO and a FIXME
for it.

But, the ability to connect to the same qemu multiple times is an issue,
qemu only accept one connection at a time (per socket).

> If you intend for each _ev to handle only one command at a time, and
> map commands 1:1 to connections, then the connection state needs to be
> in your ev structure.

I did intend to have one _ev handle one command, but we need to be able
to send multiple command within the same connection. This is because of
the way qemu handle received fds.

For example, to open a new cdrom, this action needs to happen:
-> connect, to qmp
-> send "add-fd"
<- receive back a fdset id.
-> send "blockdev-change-medium" file=$fdset (insert a new cdrom)
-> disconnect

That will work. But if we disconnect after "add-fd" (and before
change-medium), then qemu will close the fd, because it will be unused.


> TBH the API you quote about does seem very similar to the other
> libxl_ev_FOO but it is very counterintuitive that "register" is the
> function to send a qmp command.  I think some better names would help,
> even if it means they are less regular.

Maybe libxl__ev_qmp_execute() instead. It is true that my _register
behave a bit differently to other _registers, the callback will only
ever be called only once, and should call _deregister.

> [1] The ctx is not shared between threads, but it is shared between
> independent aos etc.
> 
> > 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.
> 
> That would really help, although I think what I am saying above
> may imply that they need to be unified.
> 
> > 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.
> 
> If you are sharing the qmp connection between multiple concurrent qmp
> commands then I think you need a data structure in the ctx which can
> find an existing connection given a domid.
> 
> Ian.

-- 
Anthony PERARD

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