[Xen-devel] Re: [PATCH] libxl, Introduce a QMP client

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2011-06-06 at 14:26 +0100, Anthony Perard wrote:
> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> QMP stands for QEMU Monitor Protocol and it is used to query information
> from QEMU or to control QEMU.
> 
> This implementation will ask QEMU the list of chardevice and store the
> path to serial0 in xenstored. So we will be able to use xl console with
> QEMU upstream.
> 
> In order to connect to the QMP server, a socket file is created in
> /var/run/xen/qmp-$(domid).
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

I didn't yet review this in detail but I have a few initial
questions/thoughts.

Am I correct that QMP is intended to provide a stable interface going
forwards? Or are we tying ourselves to a particular qemu version and/or
will we need version specific bits (and associated configuration stuff)
in the future?

I think we should try where possible to keep this stuff entirely within
libxl. The existing libxl event API is a bit of a mess but I think if it
were cleaned up (IanJ has a plan I think) then it would be the right
place to integrate the libxl and caller event loops.

For the time being though I think libxl should provide the fd and not
expect the caller to construct the path and open it etc. IOW
libxl_qmp_initialize should not take a socket option, it should
construct the path, do the open internally and return the fd.

> -        ret = select(fd + 1, &rfds, NULL, NULL, NULL);
> +        ret = select(fd > qmp_fd ? fd + 1 : qmp_fd + 1, &rfds, NULL, NULL, 
> NULL);
>          if (!ret)
>              continue;
> +        if (FD_ISSET(fd, &rfds)) {
>          libxl_get_event(ctx, &event);
>          switch (event.type) {
>              case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
> @@ -1687,6 +1706,10 @@ start:
>                  break;
>          }
>          libxl_free_event(&event);
> +        }
> +        if (FD_ISSET(qmp_fd, &rfds)) {
> +            libxl_qmp_do_next(qmp_handler);
> +        }

Looks like some re-indentation is needed in these two hunks?

>      }
> 
>  error_out:
> @@ -1697,6 +1720,10 @@ error_out:
>  out:
>      if (logfile != 2)
>          close(logfile);
> +    if (qmp_handler) {
> +        libxl_qmp_close(qmp_handler);
> +        qmp_handler = NULL;
> +    }

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel