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