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