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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Jul 1, 2011 at 09:39, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> On Thu, 2011-06-30 at 18:30 +0100, Anthony PERARD wrote:
>> 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).
>
> Should include "libxl" in the name somewhere to differentiate from other
> potential connections in the future?

Well, this socket belong to QEMU (the creator). So I'm not sure that
"libxl" in the name is appropriate.

But did you prefer to see "qmp-libxl-$(domid)" as a socket file name ?

[...]

>> +/*
>> + * Handler functions
>> + */
>> +
>> +static libxl__qmp_handler *qmp_init_handler(libxl_ctx *ctx, uint32_t domid)
>
> libxl__qmp_init_handler?

"libxl__" event if it's intern to this file?

> Generally internal functions should take a libxl__gc *gc rather than a
> ctx (applies to a bunch of functions).
>
>> +{
>> + Â Âlibxl__qmp_handler *qmp = NULL;
>> +
>> + Â Âqmp = calloc(1, sizeof (libxl__qmp_handler));
>
> Could be attached to the libxl__gc infrastructure instead of using an
> explicit free?

So, I should use the garbage collector for the handler, but also for
the callback ? For the callback, I know when I can free them because
they will not be used anymore. But if I use the gc, the callbacks will
not be freed before the dustman is called by the caller.

> BTW, I was wondering the same thing as I read the parser stuff in the
> previous patch but that would involve plumbing a *gc through and careful
> thought about whether or not a given data field could ever reach the
> libxl user (e.g. JSON_STRING's could ultimately get returned to the
> caller).

Well, a json_string should be strdup because, with or without gc,
everything malloc for a json_object will be free with this object.

I will try to use the gc with the json parsing stuff.

>> +static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
>> + Â Â Â Â Â Â Â Â Â Âint timeout)
>> +{
>> + Â Âint ret;
>> + Â Âint i = 0;
>> +
>> + Â Âqmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
>> + Â Âif (qmp->qmp_fd < 0) {
>> + Â Â Â Âreturn -1;
>> + Â Â}
>> +
>> + Â Âmemset(&qmp->addr, 0, sizeof (&qmp->addr));
>> + Â Âqmp->addr.sun_family = AF_UNIX;
>> + Â Âstrncpy(qmp->addr.sun_path, qmp_socket_path,
>> + Â Â Â Â Â Âsizeof (qmp->addr.sun_path));
>> +
>> + Â Âdo {
>> + Â Â Â Âret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
>> + Â Â Â Â Â Â Â Â Â Â Âsizeof (qmp->addr));
>> + Â Â Â Âif (ret == 0)
>> + Â Â Â Â Â Âbreak;
>> + Â Â Â Âif (errno == ENOENT || errno == ECONNREFUSED) {
>> + Â Â Â Â Â Â/* ENOENT Â Â Â : Socket may not have shown up yet
>> + Â Â Â Â Â Â * ECONNREFUSED : Leftover socket hasn't been removed yet */
>> + Â Â Â Â Â Âcontinue;
>> + Â Â Â Â}
>> + Â Â Â Âreturn -1;
>> + Â Â} while ((++i / 5 <= timeout) && (usleep(200 * 1000) <= 0));
>
> If we exit this loop because we timed out do we need to set errno to
> ETIMEDOUT or something similar to indicate this? Or is the existing err
> errno (one of ENOENT or ECONNREFUSED?) a sufficient indicator?

I think it's should be a ETIMEDOUT. ENOENT or ECONNREFUSED explain
just why we timeout.

>> +
>> + Â Âreturn ret;
>> +}
>> +
>> +static void qmp_close(libxl__qmp_handler *qmp)
>> +{
>> + Â Âcallback_id_pair *pp = NULL;
>> + Â Âcallback_id_pair *tmp = NULL;
>> +
>> + Â Âclose(qmp->qmp_fd);
>> + Â ÂSIMPLEQ_FOREACH(pp, &qmp->callback_list, next) {
>> + Â Â Â Âif (tmp)
>> + Â Â Â Â Â Âfree(tmp);
>> + Â Â Â Âtmp = pp;
>
> That seems like an odd construct, but I suppose it is necessary to work
> around the lack of a SIMPLEQ_FOREACH variant which is safe against
> removing the iterator from the list.

Yes.

>> +libxl__qmp_handler *libxl__qmp_initialize(libxl_ctx *ctx, uint32_t domid)
>> +{
>> + Â Âint ret = 0;
>> + Â Âlibxl__qmp_handler *qmp = NULL;
>> + Â Âchar *qmp_socket;
>> + Â Âlibxl__gc gc = LIBXL_INIT_GC(ctx);
>> +
>> + Â Âqmp = qmp_init_handler(ctx, domid);
>> +
>> + Â Âqmp_socket = libxl__sprintf(&gc, "%s/qmp-%d",
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlibxl_run_dir_path(), domid);
>> + Â Âif ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) < 0) {
>> + Â Â Â ÂLIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Connection error");
>> + Â Â Â Âqmp_free_handler(qmp);
>> + Â Â Â Âreturn NULL;
>> + Â Â}
>> +
>> + Â ÂLIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "connected to %s", qmp_socket);
>> +
>> + Â Â/* Wait for the response to qmp_capabilities */
>> + Â Âwhile (!qmp->connected) {
>> + Â Â Â Âif ((ret = qmp_next(qmp)) < 0) {
>> + Â Â Â Â Â Âbreak;
>
> Is it an error condition not to get in to the connected state? Should we
> therefore qmp_free_handler and return NULL? That would save callers
> checking qmp->connected since they can just assume it is true...

That meen that QEMU did not respond to the "qmp_capabilities" command.
And it's needed in order send other commands. So yes, it's an error. I
will return NULL.

> [...]
>> +int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid)
>> +{
>> + Â Âlibxl__qmp_handler *qmp = NULL;
>> + Â Âint ret = 0;
>> +
>> + Â Âqmp = libxl__qmp_initialize(ctx, domid);
>> + Â Âif (!qmp)
>> + Â Â Â Âreturn -1;
>> + Â Âif (qmp->connected) {
>
> ... like here.
>
>> + Â Â Â Âret = libxl__qmp_query_serial(qmp);
>> + Â Â}
>> + Â Âlibxl__qmp_close(qmp);
>> + Â Âreturn ret;
>> +}

-- 
Anthony PERARD

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