WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Anthony PERARD <anthony.perard@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH V7 7/7] libxl, Introduce a QMP client
From: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Date: Thu, 21 Jul 2011 12:17:28 +0100
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Thu, 21 Jul 2011 04:18:29 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1311197072-16003-8-git-send-email-anthony.perard@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1311197072-16003-1-git-send-email-anthony.perard@xxxxxxxxxx> <1311197072-16003-8-git-send-email-anthony.perard@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Wed, 2011-07-20 at 22:24 +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).

That path doesn't match the implementation now.

Again I think I've reviewed much of this before so I only skimmed it,
although I paid attention to the new stuff relating to pulling through
to crlf etc.

> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> new file mode 100644
> index 0000000..90dba72
> --- /dev/null
> +++ b/tools/libxl/libxl_qmp.c
[...]
> +static int register_serials_chardev_callback(libxl__qmp_handler *qmp,
> +                                             const libxl__json_object *o)
> +{
> +    const libxl__json_object *obj = NULL;
> +    const libxl__json_object *label = NULL;
> +    const char *s = NULL;
> +    flexarray_t *array = NULL;
> +    int index = 0;
> +    const char *chardev = NULL;
> +    int ret = 0;
> +
> +    if ((array = json_object_get_array(o)) == NULL) {
> +        return -1;
> +    }
> +
> +    for (index = 0; index < array->count; index++) {
> +        if (flexarray_get(array, index, (void**)&obj) != 0)
> +            break;

I think a helper function to retrieve an index into a json_array would
be helpful. I think in general exposing the internal use of flexarrays
in this interface to callers can be avoided.

> +        label = json_object_get("label", obj, JSON_STRING);
> +        s = json_object_get_string(label);

If obj is not a map label will be null but json_object_get_string will
DTRT and return NULL so this works. But perhaps an explicit type check
would be correct here?

> +/*
> + * Helpers
> + */
> +
> +static libxl__qmp_message_type qmp_response_type(libxl__qmp_handler *qmp,
> +                                                 const libxl__json_object *o)
> +{
> +    flexarray_t *maps = json_object_get_map(o);
> +    libxl__qmp_message_type type;
> +
> +    if (maps) {
> +        libxl__json_map_node *node = NULL;
> +        int index = 0;
> +
> +        for (index = 0; index < maps->count; index++) {
> +            if (flexarray_get(maps, index, (void**)&node) != 0)
> +                break;

Another helper function to get the a node from a map? Or even a
json_map_for_each_node type construct?

> +            if (libxl__qmp_message_type_from_string(node->map_key, &type) == 
> 0)
> +                return type;
> +        }
> +    }
> +
> +    return LIBXL__QMP_MESSAGE_TYPE_INVALID;
> +}
> +
> +static callback_id_pair *qmp_get_callback_from_id(libxl__qmp_handler *qmp,
> +                                                  const libxl__json_object 
> *o)
> +{
> +    const libxl__json_object *id_object = json_object_get("id", o,
> +                                                          JSON_INTEGER);
> +    int id = -1;
> +    callback_id_pair *pp = NULL;
> +
> +    if (id_object) {
> +        id = json_object_get_integer(id_object);

Check that it is an integer? Presumably the -1 error return is never a
valid id but it'd save a useless list walk.

> +        SIMPLEQ_FOREACH(pp, &qmp->callback_list, next) {
> +            if (pp->id == id) {
> +                return pp;
> +            }
> +        }
> +    }
> +    return NULL;
> +}
> +
[...]
> +static inline yajl_gen_status libxl__yajl_gen_asciiz(yajl_gen hand,
> +                                                     const char *str)
> +{
> +    return yajl_gen_string(hand, (const unsigned char *)str, strlen(str));
> +}

Belongs in libxl_json I think.

> +static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
> +{
> +    ssize_t rd;
> +    char *s = NULL;
> +    char *s_end = NULL;
> +
> +    char *incomplete = NULL;
> +    size_t incomplete_size = 0;
> +
> +    do {
> +        fd_set rfds;
> +        int ret = 0;
> +        struct timeval timeout = {
> +            .tv_sec = qmp->timeout,
> +            .tv_usec = 0,
> +        };
> +
> +        FD_ZERO(&rfds);
> +        FD_SET(qmp->qmp_fd, &rfds);
> +
> +        ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
> +        if (ret > 0) {
> +            rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);

If you do "if (rd == 0) continue" and the rd < 0 error handling+return
here the error handling is nearer the error-site and hence easier to
grok. (also you save a level of indentation, although the inner loop
isn't wrapping in this particular case)

Actually the error handling for ret = select is much the same, if you
pull the ret==0 and ret<0 cases up after the select it easier to follow.

> +            if (rd > 0) {
> +                DEBUG_REPORT_RECEIVED(qmp->buffer, rd);
> +
> +                do {
> +                    char *end = NULL;
> +                    if (incomplete) {
> +                        size_t current_pos = s - incomplete;
> +                        incomplete_size += rd;
> +                        incomplete = libxl__realloc(gc, incomplete, 
> incomplete_size + 1);
> +                        incomplete = strncat(incomplete, qmp->buffer, rd);
> +                        s = incomplete + current_pos;
> +                        s_end = incomplete + incomplete_size;
> +                    } else {
> +                        incomplete = libxl__strndup(gc, qmp->buffer, rd);
> +                        incomplete_size = rd;
> +                        s = incomplete;
> +                        s_end = s + rd;
> +                    }
> +
> +                    end = strstr(s, "\r\n");

Is s definitely NULL terminated here? (yes, according to strncat(3),
good!)

> +                    if (end) {
> +                        libxl__json_object *o = NULL;
> +
> +                        *end = '\0';
> +
> +                        o = libxl__json_parse(gc, s);
> +                        s = end + 2;
> +
> +                        if (o) {
> +                            qmp_handle_response(qmp, o);
> +                            json_object_free(qmp->ctx, o);
> +                        }

"if (!o)" is now an error since you pulled up to a "\r\n"?

> +                    } else {
> +                        break;
> +                    }
> +                } while (s < s_end);
> +            } else if (rd < 0) {
> +                LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
> +                                 "Socket read error");
> +                return rd;
> +            }
> +        } else if (ret == 0) {
> +            LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR, "timeout");
> +            return -1;
> +        } else if (ret < 0) {
> +            LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR, "Select error");
> +            return -1;
> +        }
> +    } while (s < s_end);
> +
> +    return 1;
> +}
> +
[...]

Ian.


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