On Wed, Jun 15, 2011 at 11:04, 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...
>
> 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...
Actually, we could also set up the VNC password by this way. So in
this case, it will not be anymore only "gather initial info".
>> 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?
Actually, qemu doesn't remove the socket. So we will have do clean it
when the domain is cleaned. Is libxl_domain_destroy() a good function
to do that?
> 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?)
-qmp is the easy way to enable QMP. But to have more complexe
combinations they suggest to use -chardev with -mon:
-chardev socket,id=mon1,path=./qmp,server,nowait -mon chardev=mon1,mode=control
>> 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.
As suggested by george, I could use "fp" or "floating_point", but the
second is too long so I will just rename to "fp".
>> + 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).
OK, I will do that.
>> [...]
>> +} 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?
It's more a wrong answer to this specific command ("query-chardev")
than a protocol error. As the doc says that is an array.
>> + 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?
It should be a float. But have 0.2 * 10^6 will be better to understand
that we have 0.2 second of sleep, but not power in C. If you prefere,
I will just wrote 200000, should be OK.
> In any case given you have "* 5" wouldn't "/ 5" be a bit clearer?
Yes, a little bit.
>> +
>> + 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?
I ask myself this question when I wrote this to put "return ret;" or
these two lines. I though it was a bit clearer that the function
return only -1 or 0. But "return ret;" should be OK.
> [...]
>> +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.
If yajl needs more data, we have to call qmp_next again. If I put the
whole thing in a loop, I will also move the select. Actually, this
should be a bit better, thus they will have only one select instead of
two, currently.
>> +#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?
It does not eat it when it finished to parse a response but it will
eat these after. So the loop do another round and end up with a
partiel json data and an allocated parser.
This little loop is just here to avoid doing a not necessary turn but
is not necessary
> [...]
>> +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?
Well, it was easier to cound the char in "id" than in "execute", that
why there is a ex variable. But I think the next step would to eithier
use a #define QMP_{EXECUTE,ID} or a static const char* global to the
file.
> You'd think yajl would have yajl_gen_asciiz or something.
Unfortunatly, there is no such thing, at least in the debian version
(1.?). I did not check the 2.? version of libyajl.
> (am I the only one who thinks the choice of unsigned char in the library
> is odd?)
No, me too, it just really annoying.
>> + 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?
What did you mean ?
Call write twice was easier here than call a snprintf or other.
>> + 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?
I did want an overall timeout. But I should just have a timeout for a
single "read" and do as recommended by the man. Also, I just see that
I just try again in case of timeout (with ret == 0) so I will fix
that.
>> + 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?
No reason, indeed. I will put it in the structure.
> [...]
>> +
>> +/*
>> + * 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?
will change that.
>> + 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?
This can be use if we want in the future to know the qemu event. So
know the fd outside this file can be use. But indeed, not yet. So we
will just readd we the time will come.
>> +
>> +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.
Yes, they should be usefull in future external use. And apart from the
get_fd one, are all used.
>> +
>> +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()?
Yes, will change that.
>> + if (!qmp)
>> + return -1;
>> + if (qmp->connected) {
>> + ret = libxl__qmp_send_command(qmp, QMP_COMMAND_QUERY_CHARDEV);
>> + }
>> + libxl__qmp_close(qmp);
>> + return ret;
>> +}
Thanks for the review,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home