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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jun 15, 2011 at 11:04 AM, Ian Campbell
<Ian.Campbell@xxxxxxxxxxxxx> wrote:
> On Tue, 2011-06-14 at 18:15 +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>
>> ---
>>
>> Change v2->v3:
>>   - Use of a timeout when wait for a reply from the server.
>>   - Use of a command list, a list of pair "command" + callback. It's 
>> associated
>>     with an enum.
>>   - Introduce libxl__qmp_initializations that will ask of all informations 
>> need
>>     through QMP. (It's just a rename of libxl__qmp_get_serial_console_path 
>> from
>>     the previous patch.)
>
> I would have sworn that initiali[zs]ations wasn't the plural of
> initiali[zs]e (it sounds wrong to my ear) and that it was one of those
> words with the same plural and singular form, but the Internet tells me
> I'm wrong...

I think "initialization" generally refers to one cohesive set of
actions to initialize something; so unless it's doing several
independent sets of initializations, I'd leave it singular.

(<pedantic>initialization is the noun form of the verb initialize, not
plual.</pedantic>)

>
> On the other hand libxl__qmp_domain_create works, so would something
> like libxl__qmp_gather_initial_info, the later conveys what's actually
> going on too...
>
>> Change v1->v2:
>>   - Introduction of libxl_run_dir_path(), should maybe be in another patch.
>>   - Add a new static function qmp_synchronous_send that wait the answer from
>>     the server.
>>   - QMP is know use only inside libxl, so only one command is send through 
>> the
>>     socket and after, the connection is closed.
>>
>>
>>  Config.mk                  |    1 +
>>  config/StdGNU.mk           |    2 +
>>  tools/libxl/Makefile       |    4 +
>>  tools/libxl/libxl.h        |    1 +
>>  tools/libxl/libxl_create.c |    4 +
>>  tools/libxl/libxl_dm.c     |    6 +
>>  tools/libxl/libxl_paths.c  |    4 +
>>  tools/libxl/libxl_qmp.c    |  980 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  tools/libxl/libxl_qmp.h    |   35 ++
>>  9 files changed, 1037 insertions(+), 0 deletions(-)
>>  create mode 100644 tools/libxl/libxl_qmp.c
>>  create mode 100644 tools/libxl/libxl_qmp.h
>>
>> diff --git a/Config.mk b/Config.mk
>> index aa681ae..8b11cd8 100644
>> --- a/Config.mk
>> +++ b/Config.mk
>> @@ -133,6 +133,7 @@ define buildmakevars2file-closure
>>         echo "XEN_CONFIG_DIR=\"$(XEN_CONFIG_DIR)\"" >> $(1).tmp;           \
>>         echo "XEN_SCRIPT_DIR=\"$(XEN_SCRIPT_DIR)\"" >> $(1).tmp;           \
>>         echo "XEN_LOCK_DIR=\"$(XEN_LOCK_DIR)\"" >> $(1).tmp;               \
>> +       echo "XEN_RUN_DIR=\"$(XEN_RUN_DIR)\"" >> $(1).tmp;               \
>>         if ! cmp $(1).tmp $(1); then mv -f $(1).tmp $(1); fi
>>  endef
>>
>> diff --git a/config/StdGNU.mk b/config/StdGNU.mk
>> index 25aeb4d..68fa226 100644
>> --- a/config/StdGNU.mk
>> +++ b/config/StdGNU.mk
>> @@ -52,9 +52,11 @@ PRIVATE_BINDIR = $(PRIVATE_PREFIX)/bin
>>  ifeq ($(PREFIX),/usr)
>>  CONFIG_DIR = /etc
>>  XEN_LOCK_DIR = /var/lock
>> +XEN_RUN_DIR = /var/run/xen
>>  else
>>  CONFIG_DIR = $(PREFIX)/etc
>>  XEN_LOCK_DIR = $(PREFIX)/var/lock
>> +XEN_RUN_DIR = $(PREFIX)/var/run/xen
>>  endif
>>
>>  SYSCONFIG_DIR = $(CONFIG_DIR)/$(CONFIG_LEAF_DIR)
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 84ab76f..be5445d 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -32,6 +32,9 @@ endif
>>  LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o
>>  LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o
>>
>> +LIBXL_OBJS-y += libxl_qmp.o
>> +LIBXL_LIBS += -lyajl
>> +
>>  LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
>>                         libxl_dom.o libxl_exec.o libxl_xshelp.o 
>> libxl_device.o \
>>                         libxl_internal.o libxl_utils.o libxl_uuid.o 
>> $(LIBXL_OBJS-y)
>> @@ -115,6 +118,7 @@ install: all
>>         $(INSTALL_DIR) $(DESTDIR)$(LIBDIR)
>>         $(INSTALL_DIR) $(DESTDIR)$(INCLUDEDIR)
>>         $(INSTALL_DIR) $(DESTDIR)$(BASH_COMPLETION_DIR)
>> +       $(INSTALL_DIR) $(DESTDIR)$(XEN_RUN_DIR)
>>         $(INSTALL_PROG) xl $(DESTDIR)$(SBINDIR)
>>         $(INSTALL_PROG) libxenlight.so.$(MAJOR).$(MINOR) $(DESTDIR)$(LIBDIR)
>>         ln -sf libxenlight.so.$(MAJOR).$(MINOR) 
>> $(DESTDIR)$(LIBDIR)/libxenlight.so.$(MAJOR)
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index b0471c0..c3bbe87 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -518,6 +518,7 @@ const char *libxl_xenfirmwaredir_path(void);
>>  const char *libxl_xen_config_dir_path(void);
>>  const char *libxl_xen_script_dir_path(void);
>>  const char *libxl_lock_dir_path(void);
>> +const char *libxl_run_dir_path(void);
>>
>>  #endif /* LIBXL_H */
>>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 91e2414..e818faf 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -30,6 +30,7 @@
>>  #include "libxl_utils.h"
>>  #include "libxl_internal.h"
>>  #include "flexarray.h"
>> +#include "libxl_qmp.h"
>>
>>  void libxl_domain_config_destroy(libxl_domain_config *d_config)
>>  {
>> @@ -514,6 +515,9 @@ static int do_domain_create(libxl__gc *gc, 
>> libxl_domain_config *d_config,
>>      }
>>
>>      if (dm_starting) {
>> +        if (dm_info->device_model_version == 
>> LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
>> +            libxl__qmp_initializations(ctx, domid);
>> +        }
>>          ret = libxl__confirm_device_model_startup(gc, dm_starting);
>>          if (ret < 0) {
>>              LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 5e80bc8..094226d 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -245,6 +245,12 @@ static char ** 
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>      flexarray_vappend(dm_args, dm,
>>                        "-xen-domid", libxl__sprintf(gc, "%d", info->domid), 
>> NULL);
>>
>> +    flexarray_append(dm_args, "-qmp");
>> +    flexarray_append(dm_args,
>> +                     libxl__sprintf(gc, "unix:%s/qmp-%d,server,nowait",
>> +                                    libxl_run_dir_path(),
>> +                                    info->domid));
>
> Presumably qemu will clean this socket up in the normal shutdown case.
> Do we need to do so as well to handle crashes and the like?
>
> The handling of -qmp via monitor_parse() seems to suggest that this is a
> compat_monitor option of some sort. Is the modern way to use both a
> -chardev and a -qmp? e.g.
>        -chardev socket,id=libxl-qmp,path="....",server,nowait
>        -qmp chardev=libxl-qmp
> (or is it -mon not -qmp?)
>
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> new file mode 100644
>> index 0000000..061eea6
>> --- /dev/null
>> +++ b/tools/libxl/libxl_qmp.c
>> @@ -0,0 +1,980 @@
>> +/*
>> + * Copyright (C) 2011      Citrix Ltd.
>> + * Author Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU Lesser General Public License as published
>> + * by the Free Software Foundation; version 2.1 only. with the special
>> + * exception on linking described in file LICENSE.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU Lesser General Public License for more details.
>> + */
>> +
>> +/*
>> + * This file implement a client for QMP (QEMU Monitor Protocol). For the
>> + * Specification, see in the QEMU repository.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +
>> +#include <sys/un.h>
>> +#include <sys/queue.h>
>> +
>> +#include <yajl/yajl_parse.h>
>> +#include <yajl/yajl_gen.h>
>> +
>> +#include "libxl_internal.h"
>> +#include "flexarray.h"
>> +#include "libxl_qmp.h"
>> +
>> +/* #define DEBUG_ANSWER */
>> +/* #define DEBUG_RECEIVE */
>> +
>> +/*
>> + * json_object types
>> + */
>> +
>> +typedef struct json_object json_object;
>> +typedef struct json_map_node json_map_node;
>> +typedef enum node_type node_type_e;
>> +
>> +enum node_type {
>> +    JSON_ERROR,
>> +    JSON_NULL,
>> +    JSON_BOOL,
>> +    JSON_INTEGER,
>> +    JSON_DOUBLE,
>> +    JSON_STRING,
>> +    JSON_MAP,
>> +    JSON_ARRAY
>> +};
>
> We typically use
>        typedef enum NAME {
>                ....
>        } NAME;
> in libxl so far. NAME is the same in both places, since this is an
> internal type you could omit the first (since it's really just for
> backward compat in the public interface).
>
>> +struct json_object {
>> +    node_type_e type;
>> +    union {
>> +        bool boolean;
>> +        long integer;
>> +        double floating;
>
> floating is a funny name in the context of the other members (it's an
> adjective the others are not) also the type is double not float.
>
> I guess float and double themselves are out since they are C keywords.

I guess "floating" is short for "floating point" -- in which case "fp"
might be a more suggestive name.

>
>> +        const char *string;
>> +        /* List of json_object */
>> +        flexarray_t *array;
>> +        /* List of json_map_node */
>> +        flexarray_t *map;
>> +    } u;
>> +    json_object *parent;
>> +};
>
> Similarly we tend to just do
>        typedef struct {
>                ....
>        } NAME;
> but not in this case due to the *parent member.
>
>> +struct json_map_node {
>> +    const char *map_key;
>> +    json_object *obj;
>> +};
>
> But you could follow the convention here (and a bunch of other places).
>
>> [...]
>> +} message_type_e;
>> +
>> +struct {
>
> static... ?
>
> [...]
>> +/*
>> + * JSON callbacks
>> + */
>> +
>> +static int json_callback_null(void *opaque)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    json_object *obj;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_null(qmp->g);
>> +#endif
>> +
>> +    obj = calloc(1, sizeof (json_object));
>> +    obj->type = JSON_NULL;
>> +    json_object_append_to(qmp, obj, qmp->current);
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_boolean(void *opaque, int boolean)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    json_object *obj;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_bool(qmp->g, boolean);
>> +#endif
>> +
>> +    obj = calloc(1, sizeof (json_object));
>> +    obj->type = JSON_BOOL;
>> +    obj->u.boolean = boolean;
>> +    json_object_append_to(qmp, obj, qmp->current);
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_integer(void *opaque, long value)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    json_object *obj;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_integer(qmp->g, value);
>> +#endif
>> +
>> +    obj = calloc(1, sizeof (json_object));
>> +    obj->type = JSON_INTEGER;
>> +    obj->u.integer = value;
>> +    json_object_append_to(qmp, obj, qmp->current);
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_double(void *opaque, double value)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    json_object *obj;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_double(qmp->g, value);
>> +#endif
>> +
>> +    obj = calloc(1, sizeof (json_object));
>> +    obj->type = JSON_DOUBLE;
>> +    obj->u.floating = value;
>> +    json_object_append_to(qmp, obj, qmp->current);
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_string(void *opaque, const unsigned char *str,
>> +                           unsigned int len)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    char *t = malloc(len + 1);
>> +    json_object *obj = NULL;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_string(qmp->g, str, len);
>> +#endif
>> +
>> +    strncpy(t, (const char *) str, len);
>> +    t[len] = 0;
>> +
>> +    obj = calloc(1, sizeof (json_object));
>> +    obj->type = JSON_STRING;
>> +    obj->u.string = t;
>> +
>> +    json_object_append_to(qmp, obj, qmp->current);
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_map_key(void *opaque, const unsigned char *str,
>> +                            unsigned int len)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    char *t = malloc(len + 1);
>> +    json_object *obj = qmp->current;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_string(qmp->g, str, len);
>> +#endif
>> +
>> +    strncpy(t, (const char *) str, len);
>> +    t[len] = 0;
>> +
>> +    if (obj->type == JSON_MAP) {
>> +        json_map_node *node = malloc(sizeof (json_map_node));
>> +
>> +        node->map_key = t;
>> +        node->obj = NULL;
>> +
>> +        flexarray_append(obj->u.map, node);
>> +    } else {
>> +        return 0;
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_start_map(void *opaque)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    json_object *obj = NULL;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_map_open(qmp->g);
>> +#endif
>> +
>> +    obj = calloc(1, sizeof (json_object));
>> +    if (qmp->head == NULL) {
>> +        qmp->head = obj;
>> +    }
>> +
>> +    obj->type = JSON_MAP;
>> +    obj->u.map = flexarray_make(1, 1);
>> +
>> +    json_object_append_to(qmp, obj, qmp->current);
>> +
>> +    obj->parent = qmp->current;
>> +    qmp->current = obj;
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_end_map(void *opaque)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_map_close(qmp->g);
>> +#endif
>> +
>> +    if (qmp->current) {
>> +        qmp->current = qmp->current->parent;
>> +    } else {
>> +        LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR, "no parents for a 
>> json_object");
>> +        return 0;
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_start_array(void *opaque)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +    json_object *obj = NULL;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_array_open(qmp->g);
>> +#endif
>> +
>> +    obj = calloc(1, sizeof (json_object));
>> +    if (qmp->head == NULL) {
>> +        qmp->head = obj;
>> +    }
>> +    obj->type = JSON_ARRAY;
>> +    obj->u.array = flexarray_make(1, 1);
>> +    json_object_append_to(qmp, obj, qmp->current);
>> +    qmp->current = obj;
>> +
>> +    return 1;
>> +}
>> +
>> +static int json_callback_end_array(void *opaque)
>> +{
>> +    libxl__qmp_handler *qmp = opaque;
>> +
>> +#ifdef DEBUG_ANSWER
>> +    yajl_gen_array_close(qmp->g);
>> +#endif
>> +
>> +    if (qmp->current) {
>> +        qmp->current = qmp->current->parent;
>> +    } else {
>> +        LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR, "no parents for a 
>> json_object");
>> +        return 0;
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>> +static yajl_callbacks callbacks = {
>> +    json_callback_null,
>> +    json_callback_boolean,
>> +    json_callback_integer,
>> +    json_callback_double,
>> +    NULL,
>
> This is the "number" callback? Would be useful to do
>       NULL /* Number */,
>
> Also, how come we don't need this one? (nevermind, I see in the docs
> that interger+double and number are mutually exclusive)
>
>> +    json_callback_string,
>> +    json_callback_start_map,
>> +    json_callback_map_key,
>> +    json_callback_end_map,
>> +    json_callback_start_array,
>> +    json_callback_end_array
>> +};
>> +
>> +/*
>> + * QMP callbacks functions
>> + */
>> +
>> +static const char *get_serial0_chardev(libxl__qmp_handler *qmp, const 
>> json_object *tree)
>> +{
>> +    const json_object *ret = NULL;
>> +    const json_object *obj = NULL;
>> +    const json_object *label = NULL;
>> +    const char *s = NULL;
>> +    flexarray_t *array = NULL;
>> +    int index = 0;
>> +
>> +    ret = json_object_map_get("return", tree);
>> +
>> +    if (!ret || ret->type != JSON_ARRAY) {
>
> Do these conditions represent some sort of protocol error or is it
> acceptable?
>
>> +        return NULL;
>> +    }
>> +    array = ret->u.array;
>> +    for (index = 0; index < array->count; index++) {
>> +        if (flexarray_get(array, index, (void**)&obj) != 0)
>> +            break;
>> +        label = json_object_map_get("label", obj);
>> +        s = json_object_get_string(label);
>> +
>> +        /* TODO Could replace serial0 by serial and get all serial ttys, if 
>> sevral */
>                                                                               
> several
> [...]
>
>> +static void qmp_handle_error_response(libxl__qmp_handler *qmp, json_object 
>> *resp)
>> +{
>> +    callback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
>> +    const json_object *error = json_object_map_get("error", resp);
>> +    const char *msg = json_object_get_string(json_object_map_get("desc", 
>> error));
>> +
>> +    if (pp) {
>> +        if (pp->id == qmp->wait_for_id) {
>> +            /* tell that the id have been processed */
>> +            qmp->wait_for_id = 0;
>> +        }
>> +        SIMPLEQ_REMOVE(&qmp->callback_list, pp, callback_id_pair, next);
>> +        free(pp);
>> +    }
>> +
>> +    LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
>> +               "receive an error message from QMP server: %s",
>                   received
>
>> +               msg);
>> +}
> [...]
>> +/*
>> + * Handler functions
>> + */
>> +
>> +static int qmp_connect(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 <= timeout * 5) && (usleep(.2 * 1000000) <= 0));
>
> usleep (effectively) takes an unsigned int, what typedoes .2 * x become?
>
> In any case given you have "* 5" wouldn't "/ 5" be a bit clearer?
>
>> +
>> +    if (ret == -1)
>> +        return -1;
>> +
>> +    return 0;
>
> Are values of ret other than 0 or -1 possible here? connect only returns
> 0 or -1, I think you can just return ret?
>
> [...]
>> +static int qmp_next(libxl__qmp_handler *qmp)
>> +{
>> +    yajl_status status;
>> +    ssize_t rd;
>> +    ssize_t bytes_parsed = 0;
>> +
>> +    /* read the socket */
>> +    rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
>> +    if (rd <= 0) {
>> +        /* either an error, or nothing */
>> +        return rd;
>> +    }
>
> Does this (and the following while loop) handle reads which return only
> a partial json datastructure? I'd expect to see some sort of loop around
> the whole thing and some buffer shuffling and/or offsets if so.
>
>> +#ifdef DEBUG_RECEIVE
>> +    qmp->buffer[rd] = 0;
>> +    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "received: '%s'", qmp->buffer);
>> +#endif
>> +
>> +    while (bytes_parsed < rd) {
> [...]
>> +        /* skip the CRLF of the end of a command */
>> +        while (bytes_parsed < rd && (qmp->buffer[bytes_parsed] ==
>> '\r'
>> +                                     || qmp->buffer[bytes_parsed] ==
>> '\n')) {
>> +               bytes_parsed++;
>> +        }
>
> The parser doesn't just eat \r & \n?
> [...]
>> +static int qmp_send(libxl__qmp_handler *qmp, const char *cmd, 
>> qmp_callback_t callback)
>> +{
>> +    yajl_gen_config conf = { 0, NULL };
>> +    const unsigned char *buf;
>> +    const char *ex = "execute";
>> +    unsigned int len = 0;
>> +    yajl_gen_status s;
>> +    yajl_gen hand;
>> +
>> +    hand = yajl_gen_alloc(&conf, NULL);
>> +    if (!hand) {
>> +        return -1;
>> +    }
>> +
>> +    yajl_gen_map_open(hand);
>> +    yajl_gen_string(hand, (const unsigned char *)ex, strlen(ex));
>> +    yajl_gen_string(hand, (const unsigned char *)cmd, strlen(cmd));
>> +    yajl_gen_string(hand, (const unsigned char *)"id", 2);
>
> ex is a variable and "id" is not?
>
> You'd think yajl would have yajl_gen_asciiz or something.
>
> (am I the only one who thinks the choice of unsigned char in the library
> is odd?)
>
>> +    yajl_gen_integer(hand, ++qmp->last_id_used);
>> +    yajl_gen_map_close(hand);
>> +
>> +    s = yajl_gen_get_buf(hand, &buf, &len);
>> +
>> +    if (s) {
>> +        LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
>> +                   "could not generate a qmp command");
>> +        return -1;
>> +    }
>> +
>> +    if (callback) {
>> +        callback_id_pair *elm = malloc(sizeof (callback_id_pair));
>> +        elm->id = qmp->last_id_used;
>> +        elm->callback = callback;
>> +        SIMPLEQ_INSERT_TAIL(&qmp->callback_list, elm, next);
>> +    }
>> +
>> +    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "next qmp command: '%s'", buf);
>> +
>> +    write(qmp->qmp_fd, buf, len);
>> +    write(qmp->qmp_fd, "\r\n", 2);
>
> These need to handle partial writes?
>
>> +    yajl_gen_free(hand);
>> +
>> +    return qmp->last_id_used;
>> +}
>> +
>> +static int qmp_synchronous_send(libxl__qmp_handler *qmp, const char *cmd, 
>> qmp_callback_t callback, int ask_timeout)
>> +{
>> +    int id = 0;
>> +    int ret = 0;
>> +    fd_set rfds;
>> +    struct timeval timeout;
>> +
>> +    id = qmp_send(qmp, cmd, callback);
>> +    if (id <= 0) {
>> +        return -1;
>> +    }
>> +    qmp->wait_for_id = id;
>> +
>> +    timeout.tv_sec = ask_timeout;
>> +    timeout.tv_usec = 0;
>> +
>> +    while (qmp->wait_for_id == id) {
>> +        FD_ZERO(&rfds);
>> +        FD_SET(qmp->qmp_fd, &rfds);
>> +        ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
>
> Linux modifies timeout to reflect the amount of time left, which will
> mean that the timeout shrinks as answers which aren't for us come in.
> You need to init timeout inside the loop. select(2) recommends treating
> timeout as undefined after a select().
>
> Do we want an overall timeout in case qemu never responds?
>
>> +        if (ret > 0) {
>> +            if ((ret = qmp_next(qmp)) < 0) {
>> +                return ret;
>> +            }
>> +        } else if (ret < 0) {
>> +            LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR, "Select error");
>> +            return -1;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +static libxl__qmp_handler *qmp_init_handler(libxl_ctx *ctx, uint32_t domid)
>> +{
>> +    libxl__qmp_handler *qmp = NULL;
>> +
>> +    qmp = calloc(1, sizeof (libxl__qmp_handler));
>> +    qmp->ctx = ctx;
>> +    qmp->domid = domid;
>> +    if ((qmp->buffer = malloc(QMP_RECEIVE_BUFFER_SIZE)) == NULL) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Can not allocate the 
>> reception buffer");
>> +        free(qmp);
>> +        return NULL;
>> +    }
>
> Any reason not to include buffer[QMP_RECEIVE_BUFFER_SIZE] directly in
> libxl__qmp_handler and avoid handling multiple allocations?
>
> [...]
>> +
>> +/*
>> + * API
>> + */
>> +
>> +libxl__qmp_handler *libxl__qmp_initialize(libxl_ctx *ctx, uint32_t domid)
>> +{
>> +    int ret = 0;
>> +    libxl__qmp_handler *qmp = NULL;
>> +    char qmp_socket[40];
>> +    fd_set rfds;
>> +    struct timeval timeout;
>> +
>> +    qmp = qmp_init_handler(ctx, domid);
>> +
>> +    snprintf(qmp_socket, sizeof (qmp_socket), "%s/qmp-%d",
>> +             libxl_run_dir_path(), domid);
>
> libxl__sprintf?
>
>> +    if ((ret = qmp_connect(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);
>> +
>> +    timeout.tv_sec = 5;
>> +    timeout.tv_usec = 0;
>> +
>> +    /* Wait for the response to qmp_capabilities */
>> +    while (!qmp->connected) {
>> +        FD_ZERO(&rfds);
>> +        FD_SET(qmp->qmp_fd, &rfds);
>> +        ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
>
> Same comment as before regarding timeout's value after select.
>
> [...]
>> +}
>> +
>> +int libxl__qmp_get_fd(libxl__qmp_handler *qmp)
>> +{
>> +    if (qmp)
>> +        return qmp->qmp_fd;
>> +    else
>> +        return -1;
>> +}
>
> Unused?
>
>> +
>> +int libxl__qmp_send_command(libxl__qmp_handler *qmp, libxl__qmp_command_e 
>> command)
>> +{
> [...]
>> +}
>> +
>> +int libxl__qmp_do_next(libxl__qmp_handler *qmp)
>> +{
> [...]
>> +}
>> +
>> +void libxl__qmp_close(libxl__qmp_handler *qmp)
>> +{
> [...]
>> +}
>
> These could all be static at the moment (are some of them also unused?),
> but I assume you are making them useful for future external uses which
> is ok.
>
>> +
>> +int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid)
>> +{
>> +    libxl__qmp_handler *qmp = NULL;
>> +    int ret = 0;
>> +
>> +    qmp = libxl__qmp_initialize(ctx, domid);
>
> Might ..._open() be a better name since it returns a handle and its
> counterpart is ..._close()?
>
>> +    if (!qmp)
>> +        return -1;
>> +    if (qmp->connected) {
>> +        ret = libxl__qmp_send_command(qmp, QMP_COMMAND_QUERY_CHARDEV);
>> +    }
>> +    libxl__qmp_close(qmp);
>> +    return ret;
>> +}
>
> Ian.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>

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