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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Jun 6, 2011 at 16:45, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> 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?

QMP is intended to provide a stable interface. Once a command have
been shipped, it should not been modified.

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

OK, I will do that, with a #define of the path somewhere.

>> - Â Â Â Â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?

Oops, indeed, I forget to change that.

Thanks,

-- 
Anthony PERARD

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