[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Re: [PATCH V3] libxl, Introduce a QMP client
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |