Xen devel - Jun 2011 - [PATCH V3] libxl, Introduce a QMP client

From: Anthony PERARD <anthony.perard@citrix.com>

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@citrix.com>
---

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.)

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));
+
     if (info->type == LIBXL_DOMAIN_TYPE_PV) {
         flexarray_append(dm_args, "-xen-attach");
     }
diff --git a/tools/libxl/libxl_paths.c b/tools/libxl/libxl_paths.c
index 9c2bd06..ec940e3 100644
--- a/tools/libxl/libxl_paths.c
+++ b/tools/libxl/libxl_paths.c
@@ -64,3 +64,7 @@ const char *libxl_lock_dir_path(void)
 {
     return XEN_LOCK_DIR;
 }
+const char *libxl_run_dir_path(void)
+{
+    return XEN_RUN_DIR;
+}
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@citrix.com>
+ *
+ * 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
+};
+
+struct json_object {
+    node_type_e type;
+    union {
+        bool boolean;
+        long integer;
+        double floating;
+        const char *string;
+        /* List of json_object */
+        flexarray_t *array;
+        /* List of json_map_node */
+        flexarray_t *map;
+    } u;
+    json_object *parent;
+};
+
+struct json_map_node {
+    const char *map_key;
+    json_object *obj;
+};
+
+/*
+ * QMP types & constant
+ */
+
+typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp, const json_object
*tree);
+
+typedef enum message_type {
+    QMP_ANY,
+    QMP_QMP,
+    QMP_RETURN,
+    QMP_ERROR,
+    QMP_EVENT
+} message_type_e;
+
+struct {
+    const char *name;
+    message_type_e type;
+} member_name_to_message_type[] = {
+    { "QMP", QMP_QMP },
+    { "return", QMP_RETURN },
+    { "error", QMP_ERROR },
+    { "event", QMP_EVENT },
+    { "", QMP_ANY },
+};
+
+typedef struct callback_id_pair {
+    int id;
+    qmp_callback_t callback;
+    SIMPLEQ_ENTRY(callback_id_pair) next;
+} callback_id_pair;
+
+struct libxl__qmp_handler {
+    struct sockaddr_un addr;
+    int qmp_fd;
+    bool connected;
+    /* this will be used by the synchronous send, so we know
+     * when we can stop and close the socket
+     */
+    int wait_for_id;
+
+    unsigned char *buffer;
+    yajl_handle hand;
+
+    json_object *head;
+    json_object *current;
+
+    libxl_ctx *ctx;
+    uint32_t domid;
+
+    int last_id_used;
+    SIMPLEQ_HEAD(callback_list, callback_id_pair) callback_list;
+#ifdef DEBUG_ANSWER
+    yajl_gen g;
+#endif
+};
+
+static int qmp_send(libxl__qmp_handler *qmp, const char *cmd, qmp_callback_t
callback);
+static int qmp_synchronous_send(libxl__qmp_handler *qmp, const char *cmd,
qmp_callback_t callback, int timeout);
+
+static const size_t QMP_RECEIVE_BUFFER_SIZE = 65536;
+static const int QMP_SOCKET_CONNECT_TIMEOUT = 5;
+
+/*
+ * QMP commands
+ */
+
+static int register_serial0_chardev_callback(libxl__qmp_handler *qmp, const
json_object *tree);
+
+struct qmp_command_list_t  {
+    const char *cmd;
+    qmp_callback_t callback;
+};
+
+struct qmp_command_list_t qmp_command_list[] = {
+    [QMP_COMMAND_QUERY_CHARDEV] = { "query-chardev",
register_serial0_chardev_callback },
+    [QMP_COMMAND_NUMBER] = { NULL, NULL },
+};
+
+/*
+ * json_object functions
+ */
+
+static int json_object_append_to(libxl__qmp_handler *qmp, json_object *obj,
json_object *dst)
+{
+    if (!dst)
+        return -1;
+
+    switch (dst->type) {
+    case JSON_MAP: {
+        json_map_node *last;
+
+        flexarray_get(dst->u.map, dst->u.map->count - 1,
(void**)&last);
+        last->obj = obj;
+        break;
+    }
+    case JSON_ARRAY:
+        flexarray_append(dst->u.array, obj);
+        break;
+    default:
+        LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
+                   "error, try append an object to a %i\n",
dst->type);
+        return -1;
+    }
+
+    obj->parent = dst;
+    return 0;
+}
+
+static void json_object_free(libxl__qmp_handler *qmp, json_object *obj)
+{
+    int index = 0;
+
+    if (obj == NULL)
+        return;
+    switch (obj->type) {
+    case JSON_ERROR:
+    case JSON_NULL:
+    case JSON_BOOL:
+    case JSON_INTEGER:
+    case JSON_DOUBLE:
+        break;
+    case JSON_STRING:
+        free((void*)obj->u.string);
+        break;
+    case JSON_MAP: {
+        json_map_node *node = NULL;
+
+        for (index = 0; index < obj->u.map->count; index++) {
+            if (flexarray_get(obj->u.map, index, (void**)&node) != 0)
+                break;
+            json_object_free(qmp, node->obj);
+            free((void*)node->map_key);
+            free(node);
+            node = NULL;
+        }
+        flexarray_free(obj->u.map);
+        break;
+    }
+    case JSON_ARRAY:{
+        json_object *node = NULL;
+        break;
+
+        for (index = 0; index < obj->u.array->count; index++) {
+            if (flexarray_get(obj->u.array, index, (void**)&node) != 0)
+                break;
+            json_object_free(qmp, node);
+            node = NULL;
+        }
+        flexarray_free(obj->u.array);
+        break;
+    }
+    }
+    free(obj);
+}
+
+static inline const char *json_object_get_string(const json_object *o)
+{
+    if (o && o->type == JSON_STRING) {
+        return o->u.string;
+    }
+    return NULL;
+}
+
+static const json_object *json_object_map_get(const char *key, const
json_object *o)
+{
+    flexarray_t *maps = NULL;
+    int index = 0;
+
+    if (o && o->type == JSON_MAP) {
+        json_map_node *node = NULL;
+
+        maps = o->u.map;
+        for (index = 0; index < maps->count; index++) {
+            if (flexarray_get(maps, index, (void**)&node) != 0)
+                break;
+            if (strcmp(key, node->map_key) == 0) {
+                return node->obj;
+            }
+        }
+    }
+    return NULL;
+}
+
+/*
+ * 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,
+    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) {
+        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 */
+        if (s && strcmp("serial0", s) == 0) {
+            const json_object *filename = NULL;
+            filename = json_object_map_get("filename", obj);
+            return json_object_get_string(filename);
+        }
+    };
+
+    return NULL;
+}
+
+static int register_serial0_chardev_callback(libxl__qmp_handler *qmp, const
json_object *tree)
+{
+    libxl__gc gc = LIBXL_INIT_GC(qmp->ctx);
+    char *path = NULL;
+    const char *chardev = NULL;
+    int ret = 0;
+
+    chardev = get_serial0_chardev(qmp, tree);
+    if (!(chardev && strncmp("pty:", chardev, 4) == 0)) {
+        return 1;
+    }
+
+    path = libxl__xs_get_dompath(&gc, qmp->domid);
+    path = libxl__sprintf(&gc, "%s/serial/%d/tty", path, 0);
+
+    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG,
+               "qmp serial0: %s (%s)", chardev + 4, path);
+    ret = libxl__xs_write(&gc, XBT_NULL, path, "%s", chardev +
4);
+    libxl__free_all(&gc);
+    return ret;
+}
+
+static int qmp_capabilities_callback(libxl__qmp_handler *qmp, const json_object
*tree)
+{
+    int ret = 0;
+
+    qmp->connected = true;
+
+    return ret;
+}
+
+/*
+ * QMP commands
+ */
+
+static int enable_qmp_capabilities(libxl__qmp_handler *qmp)
+{
+    return qmp_send(qmp, "qmp_capabilities",
qmp_capabilities_callback);
+}
+
+/*
+ * Helpers
+ */
+
+static message_type_e qmp_response_type(libxl__qmp_handler *qmp, json_object
*resp)
+{
+    if (resp && resp->type == JSON_MAP) {
+        flexarray_t *maps = NULL;
+        json_map_node *node = NULL;
+        int index = 0;
+
+        maps = resp->u.map;
+        for (index = 0; index < maps->count; index++) {
+            int i = 0;
+            if (flexarray_get(maps, index, (void**)&node) != 0)
+                break;
+            for (i = 0; member_name_to_message_type[i].type != QMP_ANY; i++) {
+                if (strcmp(member_name_to_message_type[i].name,
node->map_key) == 0) {
+                    return member_name_to_message_type[i].type;
+                }
+            }
+        }
+    }
+
+    return QMP_ANY;
+}
+
+static callback_id_pair *qmp_get_callback_from_id(libxl__qmp_handler *qmp,
json_object *o)
+{
+    const json_object *id_object = json_object_map_get("id", o);
+    int id = -1;
+    callback_id_pair *pp = NULL;
+
+    if (id_object && id_object->type == JSON_INTEGER) {
+        id = id_object->u.integer;
+
+        SIMPLEQ_FOREACH(pp, &qmp->callback_list, next) {
+            if (pp->id == id) {
+                return pp;
+            }
+        }
+    }
+    return NULL;
+}
+
+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",
+               msg);
+}
+
+static int qmp_handle_response(libxl__qmp_handler *qmp, json_object *resp)
+{
+    message_type_e type = QMP_ANY;
+
+    type = qmp_response_type(qmp, resp);
+    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "message type: %i",
type);
+
+    switch (type) {
+    case QMP_QMP:
+        /* On the greeting message from the server, enable qmp capabilities */
+        enable_qmp_capabilities(qmp);
+        break;
+    case QMP_RETURN: {
+        callback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
+
+        if (pp) {
+            pp->callback(qmp, resp);
+            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);
+        }
+        break;
+    }
+    case QMP_ERROR:
+        qmp_handle_error_response(qmp, resp);
+        break;
+    case QMP_EVENT:
+        break;
+    case QMP_ANY:
+        return -1;
+    }
+    return 0;
+}
+
+/*
+ * 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));
+
+    if (ret == -1)
+        return -1;
+
+    return 0;
+}
+
+static void qmp_close(libxl__qmp_handler *qmp)
+{
+    callback_id_pair *pp = NULL;
+    callback_id_pair *tmp = NULL;
+#ifdef DEBUG_ANSWER
+    if (qmp->g)
+        yajl_gen_free(qmp->g);
+#endif
+    if (qmp->hand)
+        yajl_free(qmp->hand);
+    close(qmp->qmp_fd);
+    SIMPLEQ_FOREACH(pp, &qmp->callback_list, next) {
+        if (tmp)
+            free(tmp);
+        tmp = pp;
+    }
+    if (tmp)
+        free(tmp);
+}
+
+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;
+    }
+#ifdef DEBUG_RECEIVE
+    qmp->buffer[rd] = 0;
+    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "received:
''%s''", qmp->buffer);
+#endif
+
+    while (bytes_parsed < rd) {
+#ifdef DEBUG_ANSWER
+        if (qmp->g == NULL) {
+            yajl_gen_config conf = { 1, "  " };
+            qmp->g = yajl_gen_alloc(&conf, NULL);
+        }
+#endif
+        /* parse the input */
+        if (qmp->hand == NULL) {
+            /* allow comments */
+            yajl_parser_config cfg = { 1, 1 };
+            qmp->hand = yajl_alloc(&callbacks, &cfg, NULL, qmp);
+        }
+        status = yajl_parse(qmp->hand, qmp->buffer + bytes_parsed, rd -
bytes_parsed);
+        bytes_parsed += yajl_get_bytes_consumed(qmp->hand);
+
+        /* handle the answer */
+        if (status != yajl_status_ok && status !=
yajl_status_insufficient_data) {
+            unsigned char *str = yajl_get_error(qmp->hand, 1,
qmp->buffer, rd);
+
+            LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR, "yajl error:
%s", str);
+            yajl_free_error(qmp->hand, str);
+        }
+
+        if (status == yajl_status_ok) {
+#ifdef DEBUG_ANSWER
+            const unsigned char *buf = NULL;
+            unsigned int len = 0;
+
+            yajl_gen_get_buf(qmp->g, &buf, &len);
+            LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG,
"response:\n%s", buf);
+            yajl_gen_free(qmp->g);
+            qmp->g = NULL;
+#endif
+
+            qmp_handle_response(qmp, qmp->head);
+
+            json_object_free(qmp, qmp->head);
+            qmp->head = NULL;
+            qmp->current = NULL;
+
+            yajl_free(qmp->hand);
+            qmp->hand = NULL;
+        }
+        /* 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++;
+        }
+    }
+    return 1;
+}
+
+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);
+    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);
+    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);
+        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;
+    }
+
+    SIMPLEQ_INIT(&qmp->callback_list);
+
+    return qmp;
+}
+
+static void qmp_free_handler(libxl__qmp_handler *qmp)
+{
+    if (qmp->buffer)
+        free(qmp->buffer);
+    free(qmp);
+}
+
+/*
+ * 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);
+    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);
+        if (ret > 0) {
+            if ((ret = qmp_next(qmp)) < 0) {
+                break;
+            }
+        } else if (ret < 0) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Select error");
+            break;
+        }
+    }
+
+    return qmp;
+}
+
+int libxl__qmp_get_fd(libxl__qmp_handler *qmp)
+{
+    if (qmp)
+        return qmp->qmp_fd;
+    else
+        return -1;
+}
+
+int libxl__qmp_send_command(libxl__qmp_handler *qmp, libxl__qmp_command_e
command)
+{
+    return qmp_synchronous_send(qmp, qmp_command_list[command].cmd,
qmp_command_list[command].callback, 5);
+}
+
+int libxl__qmp_do_next(libxl__qmp_handler *qmp)
+{
+    int ret;
+
+    if (!qmp)
+        return -1;
+    ret = qmp_next(qmp);
+    if (ret < 0) {
+        LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR, "QMP, read
error");
+    }
+
+    return ret;
+}
+
+void libxl__qmp_close(libxl__qmp_handler *qmp)
+{
+    if (!qmp)
+        return;
+    qmp_close(qmp);
+    qmp_free_handler(qmp);
+}
+
+int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid)
+{
+    libxl__qmp_handler *qmp = NULL;
+    int ret = 0;
+
+    qmp = libxl__qmp_initialize(ctx, domid);
+    if (!qmp)
+        return -1;
+    if (qmp->connected) {
+        ret = libxl__qmp_send_command(qmp, QMP_COMMAND_QUERY_CHARDEV);
+    }
+    libxl__qmp_close(qmp);
+    return ret;
+}
diff --git a/tools/libxl/libxl_qmp.h b/tools/libxl/libxl_qmp.h
new file mode 100644
index 0000000..d11b9ba
--- /dev/null
+++ b/tools/libxl/libxl_qmp.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2011      Citrix Ltd.
+ * Author Anthony PERARD <anthony.perard@citrix.com>
+ *
+ * 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.
+ */
+
+#include "libxl.h"
+
+typedef struct libxl__qmp_handler libxl__qmp_handler;
+typedef enum libxl__qmp_command_e libxl__qmp_command_e;
+
+/* QMP Command that can be send */
+enum libxl__qmp_command_e {
+    QMP_COMMAND_QUERY_CHARDEV,
+    QMP_COMMAND_NUMBER,
+};
+
+
+_hidden libxl__qmp_handler *libxl__qmp_initialize(libxl_ctx *ctx,
+                                          uint32_t domid);
+_hidden int libxl__qmp_get_fd(libxl__qmp_handler *qmp);
+_hidden int libxl__qmp_send_command(libxl__qmp_handler *qmp,
libxl__qmp_command_e command);
+_hidden int libxl__qmp_do_next(libxl__qmp_handler *qmp);
+_hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
+
+_hidden int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid);
-- 
1.7.2.5


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

On Tue, 2011-06-14 at 18:15 +0100, Anthony Perard wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> 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@citrix.com>
> ---
> 
> 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@citrix.com>
> + *
> + * 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@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, Jun 15, 2011 at 11:04 AM, Ian Campbell
<Ian.Campbell@eu.citrix.com> wrote:
> On Tue, 2011-06-14 at 18:15 +0100, Anthony Perard wrote:
>> From: Anthony PERARD <anthony.perard@citrix.com>
>>
>> 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@citrix.com>
>> ---
>>
>> 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@citrix.com>
>> + *
>> + * 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@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 15 Jun 2011, Ian Campbell wrote:
> > +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 qemu never responds we just need a single timeout inside the loop :)

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

On Wed, Jun 15, 2011 at 11:04, Ian Campbell <Ian.Campbell@eu.citrix.com>
wrote:
> On Tue, 2011-06-14 at 18:15 +0100, Anthony Perard wrote:
>> From: Anthony PERARD <anthony.perard@citrix.com>
>>
>> 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@citrix.com>
>> ---
>>
>> 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@citrix.com>
>> + *
>> + * 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@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, Jun 15, 2011 at 7:44 PM, Anthony PERARD
<anthony.perard@citrix.com> wrote:
>>> +    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.
I think the standard way for making things like this more readable is
something like:

#define MS (1000)
...
usleep(200 * MS);

But if there''s only one sleep, it''s probably bike-shedding
either way.

 -George

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

On Wed, 2011-06-15 at 19:44 +0100, Anthony PERARD wrote:
> On Wed, Jun 15, 2011 at 11:04, Ian Campbell
<Ian.Campbell@eu.citrix.com> wrote:
> > On Tue, 2011-06-14 at 18:15 +0100, Anthony Perard wrote:
> >> From: Anthony PERARD <anthony.perard@citrix.com>
> >>
> >> 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@citrix.com>
> >> ---
> >>
> >> 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".
Sure. In which case libxl__qmp_domain_create (or _setup) works.
> >> @@ -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?
I think so. There must be a function somewhere which signals qemu to
shutdown (right?) -- I guess that be a good place?
> >> +        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.
My concern was just the floating * int multiplication going into a
function which takes an int. I''m sure it all works out through the up-
and down-casting and constant propagation etc and turns into the right
thing, I just wondered if we could avoid it and make things clearer at
the same time.

> >> +#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
It seems worthwhile then.
> > [...]
> >> +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.
Perhaps we should have our own little helper function/macro which does
this?
> >> +    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.
I meant handling of write() returning < len or EINTR, EAGAIN etc.
> Thanks for the review,
No problem!

Ian.


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

Ian Campbell writes ("[Xen-devel] Re: [PATCH V3] libxl, Introduce a QMP
client"):
> On Wed, 2011-06-15 at 19:44 +0100, Anthony PERARD wrote:
> > 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?
> 
> I think so. There must be a function somewhere which signals qemu to
> shutdown (right?) -- I guess that be a good place?
The hard domain destruction code would be the right place.  We should
unconditionally unlink the socket there (rather than try to guess
whether it ought to have been created).
> > 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.
> 
> My concern was just the floating * int multiplication going into a
> function which takes an int. I''m sure it all works out through the
up-
> and down-casting and constant propagation etc and turns into the right
> thing, I just wondered if we could avoid it and make things clearer at
> the same time.
I would suggest  200*1000  as that doesn''t make it so hard to count
zeroes.  The function is called "usleep" so it''s clear that
the value
is in microseconds, and 200*1000 us is indeed 0.2s.
> > >> +    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);
These casts are very unpleasant but seem to be required due to the
broken yajl API.
> > > 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.
> 
> Perhaps we should have our own little helper function/macro which does
> this?
Yes.  We can hide the cast in it too.
> > >> +    if (callback) {
> > >> +        callback_id_pair *elm = malloc(sizeof
(callback_id_pair));
What if malloc fails ?
> > >> +    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.
> 
> I meant handling of write() returning < len or EINTR, EAGAIN etc.
Quite so.

Ian.

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

anthony.perard@citrix.com writes ("[Xen-devel] [PATCH V3] libxl, Introduce
a QMP client"):
> +#ifdef DEBUG_ANSWER
> +    yajl_gen_bool(qmp->g, boolean);
> +#endif
Can we not have some kind of macro setup to avoid the #ifdefs ?
      DEBUG_GEN(bool, boolean)
or something.

Ian.

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