Xen devel - Oct 2011 - [PATCH V2 00/11] libxl: QMP client improvement + pci passthrougth insert through QMP

This patch series improve the QMP client in lib XenLight to be able to insert a
PCI passthrough device with the upstream QEMU. This require to apply the patch
(RFC) series post earlier this week for QEMU.

The first three patches are fix.

The first patch create a key in xenstore with the version of the running device
model, here: /libxl/$domid/dm-version

Change with v1:
  - 3 new patches, with small fix.
  - add a structure in qmp, qmp_request_handle, to carry the return code of a
    callback.
  - the xenstore key for the dm-version is now in /libxl/$domid, instead of
    /local/domain/$domid
  - new patch to parse number bigger than LONG_MAX for json.
  - an user specified vdevfn for a pci passthrough devices is now handle.
  - in the last patch, the code to handle a pci-add through xenstore have is
    own function. that help a bit to keep the code under 80col.


Anthony PERARD (11):
  libxl_qmp, Fix return check of fcntl
  libxl_json, Check the parser status before to call parse_complete
  libxl_qmp, Better error message after a parse error.
  libxl, Introduce dm-version xenstore key.
  libxl_qmp, Introduce an opaque argument to the callbacks.
  libxl_qmp, Introduce list of arguments to qmp_send
  libxl_qmp, Always insert a command id in the callback_list.
  libxl_qmp, Introduce qmp_request_handle.
  libxl_json, Handle number abrove LONG_MAX.
  libxl_qmp, Introduce libxl__qmp_pci_add.
  libxl, Use QMP to insert a passthrough device when using upstream
    QEMU

 tools/libxl/libxl.c          |    2 +
 tools/libxl/libxl_create.c   |   28 ++++++
 tools/libxl/libxl_internal.c |   19 ++++
 tools/libxl/libxl_internal.h |   16 +++-
 tools/libxl/libxl_json.c     |   56 ++++++-----
 tools/libxl/libxl_pci.c      |   71 ++++++++++-----
 tools/libxl/libxl_qmp.c      |  214 +++++++++++++++++++++++++++++++++++-------
 7 files changed, 322 insertions(+), 84 deletions(-)

-- 
Anthony PERARD


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

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 618f20f..ef36348 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -296,7 +296,7 @@ static int qmp_open(libxl__qmp_handler *qmp, const char
*qmp_socket_path,
     if (qmp->qmp_fd < 0) {
         return -1;
     }
-    if ((flags = fcntl(qmp->qmp_fd, F_GETFL)) == 1) {
+    if ((flags = fcntl(qmp->qmp_fd, F_GETFL)) == -1) {
         flags = 0;
     }
     if (fcntl(qmp->qmp_fd, F_SETFL, flags | O_NONBLOCK) == -1) {
-- 
Anthony PERARD


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

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_json.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 11f65fc..c743114 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -744,7 +744,9 @@ libxl__json_object *libxl__json_parse(libxl__gc *gc, const
char *s)
         yajl_ctx.hand = yajl_alloc(&callbacks, &cfg, NULL,
&yajl_ctx);
     }
     status = yajl_parse(yajl_ctx.hand, (const unsigned char *)s, strlen(s));
-    status = yajl_parse_complete(yajl_ctx.hand);
+    if (status == yajl_status_ok) {
+        status = yajl_parse_complete(yajl_ctx.hand);
+    }
 
     if (status == yajl_status_ok) {
         libxl__json_object *o = yajl_ctx.head;
-- 
Anthony PERARD


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

By setting the next string to parse after having printed any error messages.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index ef36348..f61a87a 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -403,7 +403,6 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
                 *end = ''\0'';
 
                 o = libxl__json_parse(gc, s);
-                s = end + 2;
 
                 if (o) {
                     qmp_handle_response(qmp, o);
@@ -413,6 +412,8 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
                                "Parse error of : %s\n", s);
                     return -1;
                 }
+
+                s = end + 2;
             } else {
                 break;
             }
-- 
Anthony PERARD


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

The all key is /libxl/$domid/dm-version.

The /libxl/$domid dir is created with the domain and should be only accessible
by the toolstack domain.

This come with libxl__device_model_version_running helper function.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl.c          |    2 ++
 tools/libxl/libxl_create.c   |   28 ++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.c |   19 +++++++++++++++++++
 tools/libxl/libxl_internal.h |    5 +++++
 4 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 064fbc4..50b97c2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -777,6 +777,8 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid, int
force)
     if (!xs_rm(ctx->xsh, XBT_NULL, dom_path))
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs_rm failed for
%s", dom_path);
 
+    xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "/libxl/%d",
domid));
+
     libxl__userdata_destroyall(&gc, domid);
 
     rc = xc_domain_destroy(ctx->xch, domid);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 68d0fc3..bed991c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -322,6 +322,9 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
+    struct xs_permissions libxlperm[1];
+    char *libxl_path = NULL;
+
     assert(!libxl_domid_valid_guest(*domid));
 
     uuid_string = libxl__uuid2string(gc, info->uuid);
@@ -368,6 +371,15 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
         goto out;
     }
 
+    libxl_path = libxl__sprintf(gc, "/libxl/%i", *domid);
+    if (!libxl_path) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create
paths");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    libxlperm[0].id = 0;
+    libxlperm[0].perms = XS_PERM_NONE;
+
     roperm[0].id = 0;
     roperm[0].perms = XS_PERM_NONE;
     roperm[1].id = *domid;
@@ -386,6 +398,10 @@ retry_transaction:
     xs_mkdir(ctx->xsh, t, vm_path);
     xs_set_permissions(ctx->xsh, t, vm_path, roperm, ARRAY_SIZE(roperm));
 
+    xs_rm(ctx->xsh, t, libxl_path);
+    xs_mkdir(ctx->xsh, t, libxl_path);
+    xs_set_permissions(ctx->xsh, t, libxl_path, libxlperm,
ARRAY_SIZE(libxlperm));
+
     xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path),
vm_path, strlen(vm_path));
     rc = libxl__domain_rename(gc, *domid, 0, info->name, t);
     if (rc)
@@ -429,6 +445,16 @@ retry_transaction:
     return rc;
 }
 
+static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
+                             libxl_device_model_info *dm_info)
+{
+    char *path = NULL;
+
+    path = libxl__sprintf(gc, "/libxl/%i/dm-version", domid);
+    return libxl__xs_write(gc, XBT_NULL, path, libxl__strdup(gc,
+       
libxl_device_model_version_to_string(dm_info->device_model_version)));
+}
+
 static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
                             libxl_console_ready cb, void *priv,
                             uint32_t *domid_out, int restore_fd)
@@ -485,6 +511,8 @@ static int do_domain_create(libxl__gc *gc,
libxl_domain_config *d_config,
         goto error_out;
     }
 
+    store_libxl_entry(gc, domid, dm_info);
+
     for (i = 0; i < d_config->num_disks; i++) {
         ret = libxl_device_disk_add(ctx, domid, &d_config->disks[i]);
         if (ret) {
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 3993d8e..5d0a2d4 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -319,6 +319,25 @@ int libxl__fd_set_cloexec(int fd)
     return fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
+libxl_device_model_version libxl__device_model_version_running(libxl__gc *gc,
+                                                               uint32_t domid)
+{
+    char *path = NULL;
+    char *dm_version = NULL;
+    libxl_device_model_version value;
+
+    path = libxl__sprintf(gc, "/libxl/%d/dm-version", domid);
+    dm_version = libxl__xs_read(gc, XBT_NULL, path);
+    if (!dm_version) {
+        return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+    }
+
+    if (libxl_device_model_version_from_string(dm_version, &value) < 0)
{
+        return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+    }
+    return value;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2e26ac6..5720b31 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -554,6 +554,11 @@ _hidden void libxl__json_object_free(libxl__gc *gc,
libxl__json_object *obj);
 
 _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc, const char *s);
 
+  /* Based on /local/domain/$domid/dm-version xenstore key
+   * default is qemu xen traditional */
+_hidden libxl_device_model_version
+libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
+
 #endif
 
 /*
-- 
Anthony PERARD


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

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_qmp.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f61a87a..d711cca 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -43,11 +43,13 @@
 #define QMP_RECEIVE_BUFFER_SIZE 4096
 
 typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
-                              const libxl__json_object *tree);
+                              const libxl__json_object *tree,
+                              void *opaque);
 
 typedef struct callback_id_pair {
     int id;
     qmp_callback_t callback;
+    void *opaque;
     SIMPLEQ_ENTRY(callback_id_pair) next;
 } callback_id_pair;
 
@@ -70,7 +72,8 @@ struct libxl__qmp_handler {
 };
 
 static int qmp_send(libxl__qmp_handler *qmp,
-                    const char *cmd, qmp_callback_t callback);
+                    const char *cmd,
+                    qmp_callback_t callback, void *opaque);
 
 static const int QMP_SOCKET_CONNECT_TIMEOUT = 5;
 
@@ -100,7 +103,8 @@ static int store_serial_port_info(libxl__qmp_handler *qmp,
 }
 
 static int register_serials_chardev_callback(libxl__qmp_handler *qmp,
-                                             const libxl__json_object *o)
+                                             const libxl__json_object *o,
+                                             void *unused)
 {
     const libxl__json_object *obj = NULL;
     const libxl__json_object *label = NULL;
@@ -144,7 +148,7 @@ static int
register_serials_chardev_callback(libxl__qmp_handler *qmp,
 }
 
 static int qmp_capabilities_callback(libxl__qmp_handler *qmp,
-                                     const libxl__json_object *o)
+                                     const libxl__json_object *o, void *unused)
 {
     qmp->connected = true;
 
@@ -157,7 +161,7 @@ static int qmp_capabilities_callback(libxl__qmp_handler
*qmp,
 
 static int enable_qmp_capabilities(libxl__qmp_handler *qmp)
 {
-    return qmp_send(qmp, "qmp_capabilities",
qmp_capabilities_callback);
+    return qmp_send(qmp, "qmp_capabilities",
qmp_capabilities_callback, NULL);
 }
 
 /*
@@ -208,7 +212,7 @@ static void qmp_handle_error_response(libxl__qmp_handler
*qmp,
     resp = libxl__json_map_get("desc", resp, JSON_STRING);
 
     if (pp) {
-        pp->callback(qmp, NULL);
+        pp->callback(qmp, NULL, pp->opaque);
         if (pp->id == qmp->wait_for_id) {
             /* tell that the id have been processed */
             qmp->wait_for_id = 0;
@@ -241,7 +245,8 @@ static int qmp_handle_response(libxl__qmp_handler *qmp,
 
         if (pp) {
             pp->callback(qmp,
-                         libxl__json_map_get("return", resp,
JSON_ANY));
+                         libxl__json_map_get("return", resp,
JSON_ANY),
+                         pp->opaque);
             if (pp->id == qmp->wait_for_id) {
                 /* tell that the id have been processed */
                 qmp->wait_for_id = 0;
@@ -424,7 +429,8 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
 }
 
 static int qmp_send(libxl__qmp_handler *qmp,
-                    const char *cmd, qmp_callback_t callback)
+                    const char *cmd,
+                    qmp_callback_t callback, void *opaque)
 {
     yajl_gen_config conf = { 0, NULL };
     const unsigned char *buf;
@@ -462,6 +468,7 @@ static int qmp_send(libxl__qmp_handler *qmp,
         }
         elm->id = qmp->last_id_used;
         elm->callback = callback;
+        elm->opaque = opaque;
         SIMPLEQ_INSERT_TAIL(&qmp->callback_list, elm, next);
     }
 
@@ -484,13 +491,14 @@ error:
 }
 
 static int qmp_synchronous_send(libxl__qmp_handler *qmp, const char *cmd,
-                                qmp_callback_t callback, int ask_timeout)
+                                qmp_callback_t callback, void *opaque,
+                                int ask_timeout)
 {
     int id = 0;
     int ret = 0;
     libxl__gc gc = LIBXL_INIT_GC(qmp->ctx);
 
-    id = qmp_send(qmp, cmd, callback);
+    id = qmp_send(qmp, cmd, callback, opaque);
     if (id <= 0) {
         return -1;
     }
@@ -580,6 +588,7 @@ int libxl__qmp_query_serial(libxl__qmp_handler *qmp)
 {
     return qmp_synchronous_send(qmp, "query-chardev",
                                 register_serials_chardev_callback,
+                                NULL,
                                 qmp->timeout);
 }
 
-- 
Anthony PERARD


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

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_qmp.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index d711cca..8d95aae 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -72,7 +72,7 @@ struct libxl__qmp_handler {
 };
 
 static int qmp_send(libxl__qmp_handler *qmp,
-                    const char *cmd,
+                    const char *cmd, libxl_key_value_list *args,
                     qmp_callback_t callback, void *opaque);
 
 static const int QMP_SOCKET_CONNECT_TIMEOUT = 5;
@@ -161,7 +161,8 @@ static int qmp_capabilities_callback(libxl__qmp_handler
*qmp,
 
 static int enable_qmp_capabilities(libxl__qmp_handler *qmp)
 {
-    return qmp_send(qmp, "qmp_capabilities",
qmp_capabilities_callback, NULL);
+    return qmp_send(qmp, "qmp_capabilities", NULL,
+                    qmp_capabilities_callback, NULL);
 }
 
 /*
@@ -429,7 +430,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
 }
 
 static int qmp_send(libxl__qmp_handler *qmp,
-                    const char *cmd,
+                    const char *cmd, libxl_key_value_list *args,
                     qmp_callback_t callback, void *opaque)
 {
     yajl_gen_config conf = { 0, NULL };
@@ -448,6 +449,10 @@ static int qmp_send(libxl__qmp_handler *qmp,
     libxl__yajl_gen_asciiz(hand, cmd);
     libxl__yajl_gen_asciiz(hand, "id");
     yajl_gen_integer(hand, ++qmp->last_id_used);
+    if (args) {
+        libxl__yajl_gen_asciiz(hand, "arguments");
+        libxl_key_value_list_gen_json(hand, args);
+    }
     yajl_gen_map_close(hand);
 
     s = yajl_gen_get_buf(hand, &buf, &len);
@@ -491,6 +496,7 @@ error:
 }
 
 static int qmp_synchronous_send(libxl__qmp_handler *qmp, const char *cmd,
+                                libxl_key_value_list *args,
                                 qmp_callback_t callback, void *opaque,
                                 int ask_timeout)
 {
@@ -498,7 +504,7 @@ static int qmp_synchronous_send(libxl__qmp_handler *qmp,
const char *cmd,
     int ret = 0;
     libxl__gc gc = LIBXL_INIT_GC(qmp->ctx);
 
-    id = qmp_send(qmp, cmd, callback, opaque);
+    id = qmp_send(qmp, cmd, args, callback, opaque);
     if (id <= 0) {
         return -1;
     }
@@ -586,7 +592,7 @@ void libxl__qmp_cleanup(libxl__gc *gc, uint32_t domid)
 
 int libxl__qmp_query_serial(libxl__qmp_handler *qmp)
 {
-    return qmp_synchronous_send(qmp, "query-chardev",
+    return qmp_synchronous_send(qmp, "query-chardev", NULL,
                                 register_serials_chardev_callback,
                                 NULL,
                                 qmp->timeout);
-- 
Anthony PERARD


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

Because the function qmp_synchronous_send rely on the presence of the id
in the callback_list.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_qmp.c |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 8d95aae..5ea9429 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -213,7 +213,9 @@ static void qmp_handle_error_response(libxl__qmp_handler
*qmp,
     resp = libxl__json_map_get("desc", resp, JSON_STRING);
 
     if (pp) {
-        pp->callback(qmp, NULL, pp->opaque);
+        if (pp->callback) {
+            pp->callback(qmp, NULL, pp->opaque);
+        }
         if (pp->id == qmp->wait_for_id) {
             /* tell that the id have been processed */
             qmp->wait_for_id = 0;
@@ -245,9 +247,11 @@ static int qmp_handle_response(libxl__qmp_handler *qmp,
         callback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
 
         if (pp) {
-            pp->callback(qmp,
-                         libxl__json_map_get("return", resp,
JSON_ANY),
-                         pp->opaque);
+            if (pp->callback) {
+                pp->callback(qmp,
+                             libxl__json_map_get("return", resp,
JSON_ANY),
+                             pp->opaque);
+            }
             if (pp->id == qmp->wait_for_id) {
                 /* tell that the id have been processed */
                 qmp->wait_for_id = 0;
@@ -438,6 +442,7 @@ static int qmp_send(libxl__qmp_handler *qmp,
     unsigned int len = 0;
     yajl_gen_status s;
     yajl_gen hand;
+    callback_id_pair *elm = NULL;
 
     hand = yajl_gen_alloc(&conf, NULL);
     if (!hand) {
@@ -463,19 +468,16 @@ static int qmp_send(libxl__qmp_handler *qmp,
         return -1;
     }
 
-    if (callback) {
-        callback_id_pair *elm = malloc(sizeof (callback_id_pair));
-        if (elm == NULL) {
-            LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
-                             "Failed to allocate a QMP callback");
-            yajl_gen_free(hand);
-            return -1;
-        }
-        elm->id = qmp->last_id_used;
-        elm->callback = callback;
-        elm->opaque = opaque;
-        SIMPLEQ_INSERT_TAIL(&qmp->callback_list, elm, next);
+    elm = malloc(sizeof (callback_id_pair));
+    if (elm == NULL) {
+        LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
+                         "Failed to allocate a QMP callback");
+        goto error;
     }
+    elm->id = qmp->last_id_used;
+    elm->callback = callback;
+    elm->opaque = opaque;
+    SIMPLEQ_INSERT_TAIL(&qmp->callback_list, elm, next);
 
     LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "next qmp command:
''%s''", buf);
 
-- 
Anthony PERARD


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

This structure helps keep the return code of the callback, so a caller can read
it.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c |   71 +++++++++++++++++++++++++++++-----------------
 1 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 5ea9429..547cd53 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -46,10 +46,16 @@ typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
                               const libxl__json_object *tree,
                               void *opaque);
 
-typedef struct callback_id_pair {
-    int id;
+typedef struct qmp_request_handle {
     qmp_callback_t callback;
     void *opaque;
+    /* return code of the callback */
+    int rc;
+} qmp_request_handle;
+typedef struct callback_id_pair {
+    qmp_request_handle *handle;
+    bool free_it; /* tell if handle need to be freed */
+    int id;
     SIMPLEQ_ENTRY(callback_id_pair) next;
 } callback_id_pair;
 
@@ -73,7 +79,7 @@ struct libxl__qmp_handler {
 
 static int qmp_send(libxl__qmp_handler *qmp,
                     const char *cmd, libxl_key_value_list *args,
-                    qmp_callback_t callback, void *opaque);
+                    qmp_request_handle *handle, bool free_it);
 
 static const int QMP_SOCKET_CONNECT_TIMEOUT = 5;
 
@@ -161,8 +167,9 @@ static int qmp_capabilities_callback(libxl__qmp_handler
*qmp,
 
 static int enable_qmp_capabilities(libxl__qmp_handler *qmp)
 {
-    return qmp_send(qmp, "qmp_capabilities", NULL,
-                    qmp_capabilities_callback, NULL);
+    qmp_request_handle *h = calloc(1, sizeof (qmp_request_handle));
+    h->callback = qmp_capabilities_callback;
+    return qmp_send(qmp, "qmp_capabilities", NULL, h, 1);
 }
 
 /*
@@ -213,14 +220,17 @@ static void qmp_handle_error_response(libxl__qmp_handler
*qmp,
     resp = libxl__json_map_get("desc", resp, JSON_STRING);
 
     if (pp) {
-        if (pp->callback) {
-            pp->callback(qmp, NULL, pp->opaque);
+        qmp_request_handle *h = pp->handle;
+        if (h) {
+            h->rc = h->callback(qmp, NULL, h->opaque);
         }
         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);
+        if (pp->free_it)
+            free(pp->handle);
         free(pp);
     }
 
@@ -241,31 +251,33 @@ static int qmp_handle_response(libxl__qmp_handler *qmp,
     switch (type) {
     case LIBXL__QMP_MESSAGE_TYPE_QMP:
         /* On the greeting message from the server, enable QMP capabilities */
-        enable_qmp_capabilities(qmp);
-        break;
+        return enable_qmp_capabilities(qmp);
     case LIBXL__QMP_MESSAGE_TYPE_RETURN: {
         callback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
 
         if (pp) {
-            if (pp->callback) {
-                pp->callback(qmp,
+            qmp_request_handle *h = pp->handle;
+            if (h) {
+                h->rc = h->callback(qmp,
                              libxl__json_map_get("return", resp,
JSON_ANY),
-                             pp->opaque);
+                             h->opaque);
             }
             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);
+            if (pp->free_it)
+                free(pp->handle);
             free(pp);
         }
-        break;
+        return 0;
     }
     case LIBXL__QMP_MESSAGE_TYPE_ERROR:
         qmp_handle_error_response(qmp, resp);
-        break;
+        return -1;
     case LIBXL__QMP_MESSAGE_TYPE_EVENT:
-        break;
+        return 0;
     case LIBXL__QMP_MESSAGE_TYPE_INVALID:
         return -1;
     }
@@ -358,6 +370,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
 
     char *incomplete = NULL;
     size_t incomplete_size = 0;
+    int rc = 0;
 
     do {
         fd_set rfds;
@@ -415,7 +428,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
                 o = libxl__json_parse(gc, s);
 
                 if (o) {
-                    qmp_handle_response(qmp, o);
+                    rc = qmp_handle_response(qmp, o);
                     libxl__json_object_free(gc, o);
                 } else {
                     LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
@@ -430,12 +443,12 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler
*qmp)
         } while (s < s_end);
    } while (s < s_end);
 
-    return 1;
+    return rc;
 }
 
 static int qmp_send(libxl__qmp_handler *qmp,
                     const char *cmd, libxl_key_value_list *args,
-                    qmp_callback_t callback, void *opaque)
+                    qmp_request_handle *handle, bool free_it)
 {
     yajl_gen_config conf = { 0, NULL };
     const unsigned char *buf;
@@ -475,8 +488,8 @@ static int qmp_send(libxl__qmp_handler *qmp,
         goto error;
     }
     elm->id = qmp->last_id_used;
-    elm->callback = callback;
-    elm->opaque = opaque;
+    elm->handle = handle;
+    elm->free_it = free_it;
     SIMPLEQ_INSERT_TAIL(&qmp->callback_list, elm, next);
 
     LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "next qmp command:
''%s''", buf);
@@ -499,14 +512,14 @@ error:
 
 static int qmp_synchronous_send(libxl__qmp_handler *qmp, const char *cmd,
                                 libxl_key_value_list *args,
-                                qmp_callback_t callback, void *opaque,
+                                qmp_request_handle *handle,
                                 int ask_timeout)
 {
     int id = 0;
     int ret = 0;
     libxl__gc gc = LIBXL_INIT_GC(qmp->ctx);
 
-    id = qmp_send(qmp, cmd, args, callback, opaque);
+    id = qmp_send(qmp, cmd, args, handle, 0);
     if (id <= 0) {
         return -1;
     }
@@ -594,10 +607,16 @@ void libxl__qmp_cleanup(libxl__gc *gc, uint32_t domid)
 
 int libxl__qmp_query_serial(libxl__qmp_handler *qmp)
 {
-    return qmp_synchronous_send(qmp, "query-chardev", NULL,
-                                register_serials_chardev_callback,
-                                NULL,
-                                qmp->timeout);
+    int rc = 0;
+    qmp_request_handle request = {
+        .callback = register_serials_chardev_callback,
+    };
+    rc = qmp_synchronous_send(qmp, "query-chardev", NULL,
+                              &request, qmp->timeout);
+    if (rc == 0) {
+        rc = request.rc;
+    }
+    return rc;
 }
 
 int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid)
-- 
Anthony PERARD


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

The integers are now "long long" in the json_object. If strtoll failed
to
convert a string into a number, the number is stored as it (a char*).

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h |    7 +++--
 tools/libxl/libxl_json.c     |   52 +++++++++++++++++++++++------------------
 2 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5720b31..849b251 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -465,7 +465,8 @@ typedef enum {
     JSON_TRUE,
     JSON_FALSE,
     JSON_INTEGER,
-    JSON_DOUBLE,
+    /* number is store in string, it''s too big to be a long long */
+    JSON_NUMBER,
     JSON_STRING,
     JSON_MAP,
     JSON_ARRAY,
@@ -475,7 +476,7 @@ typedef enum {
 typedef struct libxl__json_object {
     libxl__json_node_type type;
     union {
-        long i;
+        long long i;
         double d;
         char *string;
         /* List of libxl__json_object */
@@ -534,7 +535,7 @@ flexarray_t *libxl__json_object_get_array(const
libxl__json_object *o)
     else
         return NULL;
 }
-static inline long libxl__json_object_get_integer(const libxl__json_object *o)
+static inline long long libxl__json_object_get_integer(const libxl__json_object
*o)
 {
     if (libxl__json_object_is_integer(o))
         return o->u.i;
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index c743114..2d8f61e 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -44,6 +44,7 @@ struct libxl__yajl_ctx {
 #  define DEBUG_GEN(ctx, type)              yajl_gen_##type(ctx->g)
 #  define DEBUG_GEN_VALUE(ctx, type, value) yajl_gen_##type(ctx->g, value)
 #  define DEBUG_GEN_STRING(ctx, str, n)     yajl_gen_string(ctx->g, str, n)
+#  define DEBUG_GEN_NUMBER(ctx, str, n)     yajl_gen_number(ctx->g, str, n)
 #  define DEBUG_GEN_REPORT(yajl_ctx) \
     do { \
         const unsigned char *buf = NULL; \
@@ -60,6 +61,7 @@ struct libxl__yajl_ctx {
 #  define DEBUG_GEN(ctx, type)                  ((void)0)
 #  define DEBUG_GEN_VALUE(ctx, type, value)     ((void)0)
 #  define DEBUG_GEN_STRING(ctx, value, lenght)  ((void)0)
+#  define DEBUG_GEN_NUMBER(ctx, value, lenght)  ((void)0)
 #  define DEBUG_GEN_REPORT(ctx)                 ((void)0)
 #endif
 
@@ -363,6 +365,7 @@ void libxl__json_object_free(libxl__gc *gc,
libxl__json_object *obj)
         return;
     switch (obj->type) {
     case JSON_STRING:
+    case JSON_NUMBER:
         free(obj->u.string);
         break;
     case JSON_MAP: {
@@ -504,35 +507,38 @@ static int json_callback_boolean(void *opaque, int
boolean)
     return 1;
 }
 
-static int json_callback_integer(void *opaque, long value)
+static int json_callback_number(void *opaque, const char *s, unsigned int len)
 {
     libxl__yajl_ctx *ctx = opaque;
-    libxl__json_object *obj;
-
-    DEBUG_GEN_VALUE(ctx, integer, value);
+    libxl__json_object *obj = NULL;
+    long long i;
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_INTEGER)) == NULL)
-        return 0;
-    obj->u.i = value;
+    /* should be replace by number */
+    DEBUG_GEN_NUMBER(ctx, s, len);
 
-    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
-        libxl__json_object_free(ctx->gc, obj);
-        return 0;
-    }
+    i = strtoll(s, NULL, 10);
 
-    return 1;
-}
+    if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE) {
+        char *t = NULL;
 
-static int json_callback_double(void *opaque, double value)
-{
-    libxl__yajl_ctx *ctx = opaque;
-    libxl__json_object *obj;
+        if ((obj = json_object_alloc(ctx->gc, JSON_NUMBER)) == NULL)
+            return 0;
 
-    DEBUG_GEN_VALUE(ctx, double, value);
+        t = malloc(len + 1);
+        if (t == NULL) {
+            LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc), LIBXL__LOG_ERROR,
+                             "Failed to allocate");
+            return 0;
+        }
+        strncpy(t, s, len);
+        t[len] = 0;
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_DOUBLE)) == NULL)
-        return 0;
-    obj->u.d = value;
+        obj->u.string = t;
+    } else {
+        if ((obj = json_object_alloc(ctx->gc, JSON_INTEGER)) == NULL)
+            return 0;
+        obj->u.i = i;
+    }
 
     if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
         libxl__json_object_free(ctx->gc, obj);
@@ -706,9 +712,9 @@ static int json_callback_end_array(void *opaque)
 static yajl_callbacks callbacks = {
     json_callback_null,
     json_callback_boolean,
-    json_callback_integer,
-    json_callback_double,
     NULL,
+    NULL,
+    json_callback_number,
     json_callback_string,
     json_callback_start_map,
     json_callback_map_key,
-- 
Anthony PERARD


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

This function insert a PCI passthrough device in qemu.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h |    4 ++
 tools/libxl/libxl_qmp.c      |  107 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 849b251..09f618f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -119,6 +119,9 @@ typedef struct {
 } libxl__device;
 
 #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
+#define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func)
& 0x07))
+#define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
+#define PCI_FUNC(devfn)         ((devfn) & 0x07)
 #define AUTO_PHP_SLOT          0x100
 #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
 #define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
@@ -444,6 +447,7 @@ _hidden libxl__qmp_handler *libxl__qmp_initialize(libxl_ctx
*ctx,
                                                   uint32_t domid);
 /* ask to QEMU the serial port information and store it in xenstore. */
 _hidden int libxl__qmp_query_serial(libxl__qmp_handler *qmp);
+_hidden int libxl__qmp_pci_add(libxl__gc *gc, int d, libxl_device_pci *pcidev);
 /* close and free the QMP handler */
 _hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
 /* remove the socket file, if the file has already been removed,
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 547cd53..bce7e16 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -41,6 +41,7 @@
  */
 
 #define QMP_RECEIVE_BUFFER_SIZE 4096
+#define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
 
 typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
                               const libxl__json_object *tree,
@@ -619,6 +620,112 @@ int libxl__qmp_query_serial(libxl__qmp_handler *qmp)
     return rc;
 }
 
+static int pci_add_callback(libxl__qmp_handler *qmp,
+                            const libxl__json_object *response, void *opaque)
+{
+    libxl_device_pci *pcidev = opaque;
+    const libxl__json_object *bus = NULL;
+    libxl__gc gc = LIBXL_INIT_GC(qmp->ctx);
+    int i, j, rc = -1;
+    char *asked_id = libxl__sprintf(&gc, PCI_PT_QDEV_ID,
+                                    pcidev->bus, pcidev->dev,
pcidev->func);
+
+    for (i = 0; (bus = libxl__json_array_get(response, i)); i++) {
+        const libxl__json_object *devices = NULL;
+        const libxl__json_object *device = NULL;
+        const libxl__json_object *o = NULL;
+        const char *id = NULL;
+
+        devices = libxl__json_map_get("devices", bus, JSON_ARRAY);
+
+        for (j = 0; (device = libxl__json_array_get(devices, j)); j++) {
+             o = libxl__json_map_get("qdev_id", device, JSON_STRING);
+             id = libxl__json_object_get_string(o);
+
+             if (id && strcmp(asked_id, id) == 0) {
+                 int dev_slot, dev_func;
+
+                 o = libxl__json_map_get("slot", device,
JSON_INTEGER);
+                 if (!o)
+                     goto out;
+                 dev_slot = libxl__json_object_get_integer(o);
+                 o = libxl__json_map_get("function", device,
JSON_INTEGER);
+                 if (!o)
+                     goto out;
+                 dev_func = libxl__json_object_get_integer(o);
+
+                 pcidev->vdevfn = PCI_DEVFN(dev_slot, dev_func);
+
+                 rc = 0;
+                 goto out;
+             }
+        }
+    }
+
+
+out:
+    libxl__free_all(&gc);
+    return rc;
+}
+
+int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
+{
+    libxl__qmp_handler *qmp = NULL;
+    flexarray_t *parameters = NULL;
+    libxl_key_value_list args = NULL;
+    char *hostaddr = NULL;
+    int rc = 0;
+    qmp_request_handle request = {
+        .callback = pci_add_callback,
+        .opaque = pcidev,
+    };
+
+    qmp = libxl__qmp_initialize(libxl__gc_owner(gc), domid);
+    if (!qmp)
+        return -1;
+
+    hostaddr = libxl__sprintf(gc, "%04x:%02x:%02x.%01x",
pcidev->domain,
+                              pcidev->bus, pcidev->dev, pcidev->func);
+    if (!hostaddr)
+        return -1;
+
+    parameters = flexarray_make(6, 1);
+    flexarray_append_pair(parameters, "driver",
"xen-pci-passthrough");
+    flexarray_append_pair(parameters, "id",
+                          libxl__sprintf(gc, PCI_PT_QDEV_ID,
+                                         pcidev->bus, pcidev->dev,
+                                         pcidev->func));
+    flexarray_append_pair(parameters, "hostaddr", hostaddr);
+    if (pcidev->vdevfn) {
+        flexarray_append_pair(parameters, "addr",
+                              libxl__sprintf(gc, "%x.%x",
+                                             PCI_SLOT(pcidev->vdevfn),
+                                             PCI_FUNC(pcidev->vdevfn)));
+    }
+    args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
+    if (!args)
+        return -1;
+
+    rc = qmp_synchronous_send(qmp, "device_add", &args, NULL,
qmp->timeout);
+    if (rc == 0) {
+        rc = qmp_synchronous_send(qmp, "query-pci", NULL,
+                                  &request, qmp->timeout);
+        if (rc == 0) {
+            rc = request.rc;
+        }
+    }
+
+    flexarray_free(parameters);
+    libxl__qmp_close(qmp);
+    return rc;
+}
+
 int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid)
 {
     libxl__qmp_handler *qmp = NULL;
-- 
Anthony PERARD


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

Also move the xenstore specif code to a new function and add a message if
sscanf fail.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_pci.c |   71 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 842d5b0..b18e481 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -599,11 +599,52 @@ static int pci_ins_check(libxl__gc *gc, uint32_t domid,
const char *state, void
     return 1;
 }
 
-static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev,
int starting)
+static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
+                                 libxl_device_pci *pcidev)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
+    int rc = 0;
     char *path;
     char *state, *vdevfn;
+
+    path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/state", domid);
+    state = libxl__xs_read(gc, XBT_NULL, path);
+    path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/parameter",
+                          domid);
+    if (pcidev->vdevfn) {
+        libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN,
+                        pcidev->domain, pcidev->bus, pcidev->dev,
+                        pcidev->func, pcidev->vdevfn);
+    } else {
+        libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
+                        pcidev->bus, pcidev->dev, pcidev->func);
+    }
+    path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/command",
+                          domid);
+    xs_write(ctx->xsh, XBT_NULL, path, "pci-ins",
strlen("pci-ins"));
+    rc = libxl__wait_for_device_model(gc, domid, NULL, NULL,
+                                      pci_ins_check, state);
+    path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/parameter",
+                          domid);
+    vdevfn = libxl__xs_read(gc, XBT_NULL, path);
+    path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/state",
+                          domid);
+    if ( rc < 0 )
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "qemu refused to add device: %s", vdevfn);
+    else if ( sscanf(vdevfn, "0x%x", &pcidev->vdevfn) != 1 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "wrong format for the vdevfn:
''%s''", vdevfn);
+        rc = -1;
+    }
+    xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
+
+    return rc;
+}
+
+static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev,
int starting)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
     int rc, hvm = 0;
 
     switch (libxl__domain_type(gc, domid)) {
@@ -613,27 +654,13 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, i
                                          NULL, NULL, NULL) < 0) {
             return ERROR_FAIL;
         }
-        path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/state", domid);
-        state = libxl__xs_read(gc, XBT_NULL, path);
-        path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/parameter", domid);
-        if (pcidev->vdevfn)
-            libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN,
pcidev->domain,
-                           pcidev->bus, pcidev->dev, pcidev->func,
pcidev->vdevfn);
-        else
-            libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
-                           pcidev->bus, pcidev->dev, pcidev->func);
-        path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/command", domid);
-        xs_write(ctx->xsh, XBT_NULL, path, "pci-ins",
strlen("pci-ins"));
-        rc = libxl__wait_for_device_model(gc, domid, NULL, NULL,
-                                          pci_ins_check, state);
-        path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/parameter", domid);
-        vdevfn = libxl__xs_read(gc, XBT_NULL, path);
-        path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/state", domid);
-        if ( rc < 0 )
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "qemu refused to add device:
%s", vdevfn);
-        else if ( sscanf(vdevfn, "0x%x", &pcidev->vdevfn) != 1
)
-            rc = -1;
-        xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
+        switch (libxl__device_model_version_running(gc, domid)) {
+            case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                rc = libxl__qmp_pci_add(gc, domid, pcidev);
+                break;
+            default:
+                rc = qemu_pci_add_xenstore(gc, domid, pcidev);
+        }
         if ( rc )
             return ERROR_FAIL;
         break;
-- 
Anthony PERARD


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

On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_json.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index 11f65fc..c743114 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -744,7 +744,9 @@ libxl__json_object *libxl__json_parse(libxl__gc *gc,
const char *s)
>          yajl_ctx.hand = yajl_alloc(&callbacks, &cfg, NULL,
&yajl_ctx);
>      }
>      status = yajl_parse(yajl_ctx.hand, (const unsigned char *)s,
strlen(s));
> -    status = yajl_parse_complete(yajl_ctx.hand);
> +    if (status == yajl_status_ok) {
> +        status = yajl_parse_complete(yajl_ctx.hand);
> +    }
>  
>      if (status == yajl_status_ok) {
You now have two of these checks back-to-back. I guess they could be
combined?
>          libxl__json_object *o = yajl_ctx.head;


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

On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
> The integers are now "long long" in the json_object. If strtoll
failed to
> convert a string into a number, the number is stored as it (a char*).
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |    7 +++--
>  tools/libxl/libxl_json.c     |   52
+++++++++++++++++++++++------------------
>  2 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 5720b31..849b251 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -465,7 +465,8 @@ typedef enum {
>      JSON_TRUE,
>      JSON_FALSE,
>      JSON_INTEGER,
> -    JSON_DOUBLE,
Did you accidentally remove this ...
> +    /* number is store in string, it''s too big to be a long long
*/
> +    JSON_NUMBER,
>      JSON_STRING,
>      JSON_MAP,
>      JSON_ARRAY,
> @@ -475,7 +476,7 @@ typedef enum {
>  typedef struct libxl__json_object {
>      libxl__json_node_type type;
>      union {
> -        long i;
> +        long long i;
>          double d;
... or accidentally leave this?
>          char *string;
>          /* List of libxl__json_object */
> @@ -534,7 +535,7 @@ flexarray_t *libxl__json_object_get_array(const
libxl__json_object *o)
>      else
>          return NULL;
>  }
> -static inline long libxl__json_object_get_integer(const libxl__json_object
*o)
> +static inline long long libxl__json_object_get_integer(const
libxl__json_object *o)
>  {
>      if (libxl__json_object_is_integer(o))
>          return o->u.i;
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index c743114..2d8f61e 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -44,6 +44,7 @@ struct libxl__yajl_ctx {
>  #  define DEBUG_GEN(ctx, type)              yajl_gen_##type(ctx->g)
>  #  define DEBUG_GEN_VALUE(ctx, type, value) yajl_gen_##type(ctx->g,
value)
>  #  define DEBUG_GEN_STRING(ctx, str, n)     yajl_gen_string(ctx->g,
str, n)
> +#  define DEBUG_GEN_NUMBER(ctx, str, n)     yajl_gen_number(ctx->g,
str, n)
>  #  define DEBUG_GEN_REPORT(yajl_ctx) \
>      do { \
>          const unsigned char *buf = NULL; \
> @@ -60,6 +61,7 @@ struct libxl__yajl_ctx {
>  #  define DEBUG_GEN(ctx, type)                  ((void)0)
>  #  define DEBUG_GEN_VALUE(ctx, type, value)     ((void)0)
>  #  define DEBUG_GEN_STRING(ctx, value, lenght)  ((void)0)
> +#  define DEBUG_GEN_NUMBER(ctx, value, lenght)  ((void)0)
that typo got propagated...
>  #  define DEBUG_GEN_REPORT(ctx)                 ((void)0)
>  #endif
>  
> @@ -363,6 +365,7 @@ void libxl__json_object_free(libxl__gc *gc,
libxl__json_object *obj)
>          return;
>      switch (obj->type) {
>      case JSON_STRING:
> +    case JSON_NUMBER:
>          free(obj->u.string);
>          break;
>      case JSON_MAP: {
> @@ -504,35 +507,38 @@ static int json_callback_boolean(void *opaque, int
boolean)
>      return 1;
>  }
>  
> -static int json_callback_integer(void *opaque, long value)
> +static int json_callback_number(void *opaque, const char *s, unsigned int
len)
>  {
>      libxl__yajl_ctx *ctx = opaque;
> -    libxl__json_object *obj;
> -
> -    DEBUG_GEN_VALUE(ctx, integer, value);
> +    libxl__json_object *obj = NULL;
> +    long long i;
>  
> -    if ((obj = json_object_alloc(ctx->gc, JSON_INTEGER)) == NULL)
> -        return 0;
> -    obj->u.i = value;
> +    /* should be replace by number */
> +    DEBUG_GEN_NUMBER(ctx, s, len);
>  
> -    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> -        libxl__json_object_free(ctx->gc, obj);
> -        return 0;
> -    }
> +    i = strtoll(s, NULL, 10);
>  
> -    return 1;
> -}
> +    if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE) {
> +        char *t = NULL;
>  
> -static int json_callback_double(void *opaque, double value)
> -{
> -    libxl__yajl_ctx *ctx = opaque;
> -    libxl__json_object *obj;
> +        if ((obj = json_object_alloc(ctx->gc, JSON_NUMBER)) == NULL)
> +            return 0;
>  
> -    DEBUG_GEN_VALUE(ctx, double, value);
> +        t = malloc(len + 1);
> +        if (t == NULL) {
> +            LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc),
LIBXL__LOG_ERROR,
> +                             "Failed to allocate");
> +            return 0;
> +        }
> +        strncpy(t, s, len);
> +        t[len] = 0;
>  
> -    if ((obj = json_object_alloc(ctx->gc, JSON_DOUBLE)) == NULL)
> -        return 0;
> -    obj->u.d = value;
> +        obj->u.string = t;
> +    } else {
> +        if ((obj = json_object_alloc(ctx->gc, JSON_INTEGER)) == NULL)
> +            return 0;
> +        obj->u.i = i;
> +    }
>  
>      if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
>          libxl__json_object_free(ctx->gc, obj);
> @@ -706,9 +712,9 @@ static int json_callback_end_array(void *opaque)
>  static yajl_callbacks callbacks = {
>      json_callback_null,
>      json_callback_boolean,
> -    json_callback_integer,
> -    json_callback_double,
>      NULL,
> +    NULL,
> +    json_callback_number,
>      json_callback_string,
>      json_callback_start_map,
>      json_callback_map_key,


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

On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
> The all key is /libxl/$domid/dm-version.
> 
> The /libxl/$domid dir is created with the domain and should be only
accessible
> by the toolstack domain.
> 
> This come with libxl__device_model_version_running helper function.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl.c          |    2 ++
>  tools/libxl/libxl_create.c   |   28 ++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.c |   19 +++++++++++++++++++
>  tools/libxl/libxl_internal.h |    5 +++++
>  4 files changed, 54 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 064fbc4..50b97c2 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -777,6 +777,8 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t
domid, int force)
>      if (!xs_rm(ctx->xsh, XBT_NULL, dom_path))
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs_rm failed for
%s", dom_path);
>  
> +    xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(&gc,
"/libxl/%d", domid));
> +
>      libxl__userdata_destroyall(&gc, domid);
>  
>      rc = xc_domain_destroy(ctx->xch, domid);
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 68d0fc3..bed991c 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -322,6 +322,9 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
>      xs_transaction_t t = 0;
>      xen_domain_handle_t handle;
>  
> +    struct xs_permissions libxlperm[1];
> +    char *libxl_path = NULL;
> +
The variable declaration block has little sections of perms and *_path
-- you might as well include those there. libxlperm might be better
called noperm (assuming that really is it''s meaning, xs''s
permission
scheme confuses the hell out of me).
>      assert(!libxl_domid_valid_guest(*domid));
>  
>      uuid_string = libxl__uuid2string(gc, info->uuid);
> @@ -368,6 +371,15 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
>          goto out;
>      }
>  
> +    libxl_path = libxl__sprintf(gc, "/libxl/%i", *domid);
This is probably worthy of a little helper, similar to
libxl__xs_get_dom_path(). (/vm/blah ought to have one too but doesn''t).
> +    if (!libxl_path) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create
paths");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    libxlperm[0].id = 0;
> +    libxlperm[0].perms = XS_PERM_NONE;
> +
>      roperm[0].id = 0;
>      roperm[0].perms = XS_PERM_NONE;
>      roperm[1].id = *domid;
> @@ -386,6 +398,10 @@ retry_transaction:
>      xs_mkdir(ctx->xsh, t, vm_path);
>      xs_set_permissions(ctx->xsh, t, vm_path, roperm,
ARRAY_SIZE(roperm));
>  
> +    xs_rm(ctx->xsh, t, libxl_path);
> +    xs_mkdir(ctx->xsh, t, libxl_path);
> +    xs_set_permissions(ctx->xsh, t, libxl_path, libxlperm,
ARRAY_SIZE(libxlperm));
> +
>      xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm",
dom_path), vm_path, strlen(vm_path));
>      rc = libxl__domain_rename(gc, *domid, 0, info->name, t);
>      if (rc)
> @@ -429,6 +445,16 @@ retry_transaction:
>      return rc;
>  }
>  
> +static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
> +                             libxl_device_model_info *dm_info)
> +{
> +    char *path = NULL;
> +
> +    path = libxl__sprintf(gc, "/libxl/%i/dm-version", domid);
> +    return libxl__xs_write(gc, XBT_NULL, path, libxl__strdup(gc,
> +       
libxl_device_model_version_to_string(dm_info->device_model_version)));
> +}
> +
>  static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>                              libxl_console_ready cb, void *priv,
>                              uint32_t *domid_out, int restore_fd)
> @@ -485,6 +511,8 @@ static int do_domain_create(libxl__gc *gc,
libxl_domain_config *d_config,
>          goto error_out;
>      }
>  
> +    store_libxl_entry(gc, domid, dm_info);
> +
>      for (i = 0; i < d_config->num_disks; i++) {
>          ret = libxl_device_disk_add(ctx, domid,
&d_config->disks[i]);
>          if (ret) {
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 3993d8e..5d0a2d4 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -319,6 +319,25 @@ int libxl__fd_set_cloexec(int fd)
>      return fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>  }
>  
> +libxl_device_model_version libxl__device_model_version_running(libxl__gc
*gc,
> +                                                               uint32_t
domid)
> +{
> +    char *path = NULL;
> +    char *dm_version = NULL;
> +    libxl_device_model_version value;
> +
> +    path = libxl__sprintf(gc, "/libxl/%d/dm-version", domid);
> +    dm_version = libxl__xs_read(gc, XBT_NULL, path);
> +    if (!dm_version) {
> +        return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> +    }
> +
> +    if (libxl_device_model_version_from_string(dm_version, &value)
< 0) {
I think this should be a fatal error, if you''ve managed to read
something from this key and it isn''t one of the expect values then
something bad has happened.
> +        return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> +    }
> +    return value;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 2e26ac6..5720b31 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -554,6 +554,11 @@ _hidden void libxl__json_object_free(libxl__gc *gc,
libxl__json_object *obj);
>  
>  _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc, const char
*s);
>  
> +  /* Based on /local/domain/$domid/dm-version xenstore key
> +   * default is qemu xen traditional */
> +_hidden libxl_device_model_version
> +libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
> +
>  #endif
>  
>  /*


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

On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
> This function insert a PCI passthrough device in qemu.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |    4 ++
>  tools/libxl/libxl_qmp.c      |  107
++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 849b251..09f618f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -119,6 +119,9 @@ typedef struct {
>  } libxl__device;
>  
>  #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
> +#define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) |
((func) & 0x07))
> +#define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
> +#define PCI_FUNC(devfn)         ((devfn) & 0x07)
>  #define AUTO_PHP_SLOT          0x100
>  #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
>  #define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
> @@ -444,6 +447,7 @@ _hidden libxl__qmp_handler
*libxl__qmp_initialize(libxl_ctx *ctx,
>                                                    uint32_t domid);
>  /* ask to QEMU the serial port information and store it in xenstore. */
>  _hidden int libxl__qmp_query_serial(libxl__qmp_handler *qmp);
> +_hidden int libxl__qmp_pci_add(libxl__gc *gc, int d, libxl_device_pci
*pcidev);
>  /* close and free the QMP handler */
>  _hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
>  /* remove the socket file, if the file has already been removed,
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 547cd53..bce7e16 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -41,6 +41,7 @@
>   */
>  
>  #define QMP_RECEIVE_BUFFER_SIZE 4096
> +#define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
>  
>  typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
>                                const libxl__json_object *tree,
> @@ -619,6 +620,112 @@ int libxl__qmp_query_serial(libxl__qmp_handler *qmp)
>      return rc;
>  }
>  
> +static int pci_add_callback(libxl__qmp_handler *qmp,
> +                            const libxl__json_object *response, void
*opaque)
> +{
> +    libxl_device_pci *pcidev = opaque;
> +    const libxl__json_object *bus = NULL;
> +    libxl__gc gc = LIBXL_INIT_GC(qmp->ctx);
> +    int i, j, rc = -1;
> +    char *asked_id = libxl__sprintf(&gc, PCI_PT_QDEV_ID,
> +                                    pcidev->bus, pcidev->dev,
pcidev->func);
> +
> +    for (i = 0; (bus = libxl__json_array_get(response, i)); i++) {
> +        const libxl__json_object *devices = NULL;
> +        const libxl__json_object *device = NULL;
> +        const libxl__json_object *o = NULL;
> +        const char *id = NULL;
> +
> +        devices = libxl__json_map_get("devices", bus,
JSON_ARRAY);
> +
> +        for (j = 0; (device = libxl__json_array_get(devices, j)); j++) {
> +             o = libxl__json_map_get("qdev_id", device,
JSON_STRING);
> +             id = libxl__json_object_get_string(o);
> +
> +             if (id && strcmp(asked_id, id) == 0) {
> +                 int dev_slot, dev_func;
> +
> +                 o = libxl__json_map_get("slot", device,
JSON_INTEGER);
> +                 if (!o)
> +                     goto out;
> +                 dev_slot = libxl__json_object_get_integer(o);
> +                 o = libxl__json_map_get("function", device,
JSON_INTEGER);
> +                 if (!o)
> +                     goto out;
> +                 dev_func = libxl__json_object_get_integer(o);
> +
> +                 pcidev->vdevfn = PCI_DEVFN(dev_slot, dev_func);
> +
> +                 rc = 0;
> +                 goto out;
> +             }
> +        }
> +    }
> +
> +
> +out:
> +    libxl__free_all(&gc);
> +    return rc;
> +}
> +
> +int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
> +{
> +    libxl__qmp_handler *qmp = NULL;
> +    flexarray_t *parameters = NULL;
> +    libxl_key_value_list args = NULL;
> +    char *hostaddr = NULL;
> +    int rc = 0;
> +    qmp_request_handle request = {
> +        .callback = pci_add_callback,
> +        .opaque = pcidev,
> +    };
> +
> +    qmp = libxl__qmp_initialize(libxl__gc_owner(gc), domid);
> +    if (!qmp)
> +        return -1;
> +
> +    hostaddr = libxl__sprintf(gc, "%04x:%02x:%02x.%01x",
pcidev->domain,
> +                              pcidev->bus, pcidev->dev,
pcidev->func);
> +    if (!hostaddr)
> +        return -1;
> +
> +    parameters = flexarray_make(6, 1);
> +    flexarray_append_pair(parameters, "driver",
"xen-pci-passthrough");
> +    flexarray_append_pair(parameters, "id",
> +                          libxl__sprintf(gc, PCI_PT_QDEV_ID,
> +                                         pcidev->bus, pcidev->dev,
> +                                         pcidev->func));
> +    flexarray_append_pair(parameters, "hostaddr", hostaddr);
> +    if (pcidev->vdevfn) {
> +        flexarray_append_pair(parameters, "addr",
> +                              libxl__sprintf(gc, "%x.%x",
> +                                             PCI_SLOT(pcidev->vdevfn),
> +                                            
PCI_FUNC(pcidev->vdevfn)));
> +    }
> +    args = libxl__xs_kvs_of_flexarray(gc, parameters,
parameters->count);
> +    if (!args)
> +        return -1;
> +
> +    rc = qmp_synchronous_send(qmp, "device_add", &args,
NULL, qmp->timeout);
> +    if (rc == 0) {
> +        rc = qmp_synchronous_send(qmp, "query-pci", NULL,
> +                                  &request, qmp->timeout);
> +        if (rc == 0) {
> +            rc = request.rc;
> +        }
Is every caller going to want this behaviour? Perhaps it belongs in
qmp_sync..._send?
> +    }
> +
> +    flexarray_free(parameters);
> +    libxl__qmp_close(qmp);
> +    return rc;
> +}
> +
>  int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid)
>  {
>      libxl__qmp_handler *qmp = NULL;


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

On Mon, Oct 24, 2011 at 10:38, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  tools/libxl/libxl_json.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
>> index 11f65fc..c743114 100644
>> --- a/tools/libxl/libxl_json.c
>> +++ b/tools/libxl/libxl_json.c
>> @@ -744,7 +744,9 @@ libxl__json_object *libxl__json_parse(libxl__gc
*gc, const char *s)
>>          yajl_ctx.hand = yajl_alloc(&callbacks, &cfg, NULL,
&yajl_ctx);
>>      }
>>      status = yajl_parse(yajl_ctx.hand, (const unsigned char *)s,
strlen(s));
>> -    status = yajl_parse_complete(yajl_ctx.hand);
>> +    if (status == yajl_status_ok) {
>> +        status = yajl_parse_complete(yajl_ctx.hand);
>> +    }
>>
>>      if (status == yajl_status_ok) {
>
> You now have two of these checks back-to-back. I guess they could be
> combined?
:(, I do not see how I could combine them. Because, if I call
parse_complete() after a parse() fail, I do not see a good error
message(yajl forget a bit of his status). And parse_complete() could
fail too, so I want to check is status too.
So, I check the same status twice, only in case of error.

(the second if(status) will print the error messages.)

-- 
Anthony PERARD

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

On Mon, 2011-10-24 at 15:59 +0100, Anthony PERARD wrote:
> On Mon, Oct 24, 2011 at 10:38, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> > On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
> >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> >> ---
> >>  tools/libxl/libxl_json.c |    4 +++-
> >>  1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> >> index 11f65fc..c743114 100644
> >> --- a/tools/libxl/libxl_json.c
> >> +++ b/tools/libxl/libxl_json.c
> >> @@ -744,7 +744,9 @@ libxl__json_object
*libxl__json_parse(libxl__gc *gc, const char *s)
> >>          yajl_ctx.hand = yajl_alloc(&callbacks, &cfg,
NULL, &yajl_ctx);
> >>      }
> >>      status = yajl_parse(yajl_ctx.hand, (const unsigned char *)s,
strlen(s));
> >> -    status = yajl_parse_complete(yajl_ctx.hand);
> >> +    if (status == yajl_status_ok) {
> >> +        status = yajl_parse_complete(yajl_ctx.hand);
> >> +    }
> >>
> >>      if (status == yajl_status_ok) {
> >
> > You now have two of these checks back-to-back. I guess they could be
> > combined?
> 
> :(, I do not see how I could combine them. Because, if I call
> parse_complete() after a parse() fail, I do not see a good error
> message(yajl forget a bit of his status). And parse_complete() could
> fail too, so I want to check is status too.
> So, I check the same status twice, only in case of error.
> 
> (the second if(status) will print the error messages.)
I totally missed that status was updated inside the first if, sorry!

Ian.




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

On Mon, Oct 24, 2011 at 10:57, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
>> The integers are now "long long" in the json_object. If
strtoll failed to
>> convert a string into a number, the number is stored as it (a char*).
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  tools/libxl/libxl_internal.h |    7 +++--
>>  tools/libxl/libxl_json.c     |   52
+++++++++++++++++++++++------------------
>>  2 files changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_internal.h
b/tools/libxl/libxl_internal.h
>> index 5720b31..849b251 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -465,7 +465,8 @@ typedef enum {
>>      JSON_TRUE,
>>      JSON_FALSE,
>>      JSON_INTEGER,
>> -    JSON_DOUBLE,
>
> Did you accidentally remove this ...
>
>> +    /* number is store in string, it''s too big to be a long
long */
>> +    JSON_NUMBER,
>>      JSON_STRING,
>>      JSON_MAP,
>>      JSON_ARRAY,
>> @@ -475,7 +476,7 @@ typedef enum {
>>  typedef struct libxl__json_object {
>>      libxl__json_node_type type;
>>      union {
>> -        long i;
>> +        long long i;
>>          double d;
>
> ... or accidentally leave this?
I''ve accidentally leave this double, because I do not handle float
number as I do''nt need them yet. But I probably should parse them as
well, and keep double in the structure.
>>          char *string;
>>          /* List of libxl__json_object */
>> @@ -534,7 +535,7 @@ flexarray_t *libxl__json_object_get_array(const
libxl__json_object *o)
>>      else
>>          return NULL;
>>  }
>> -static inline long libxl__json_object_get_integer(const
libxl__json_object *o)
>> +static inline long long libxl__json_object_get_integer(const
libxl__json_object *o)
>>  {
>>      if (libxl__json_object_is_integer(o))
>>          return o->u.i;
>> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
>> index c743114..2d8f61e 100644
>> --- a/tools/libxl/libxl_json.c
>> +++ b/tools/libxl/libxl_json.c
>> @@ -44,6 +44,7 @@ struct libxl__yajl_ctx {
>>  #  define DEBUG_GEN(ctx, type)              yajl_gen_##type(ctx->g)
>>  #  define DEBUG_GEN_VALUE(ctx, type, value) yajl_gen_##type(ctx->g,
value)
>>  #  define DEBUG_GEN_STRING(ctx, str, n)     yajl_gen_string(ctx->g,
str, n)
>> +#  define DEBUG_GEN_NUMBER(ctx, str, n)     yajl_gen_number(ctx->g,
str, n)
>>  #  define DEBUG_GEN_REPORT(yajl_ctx) \
>>      do { \
>>          const unsigned char *buf = NULL; \
>> @@ -60,6 +61,7 @@ struct libxl__yajl_ctx {
>>  #  define DEBUG_GEN(ctx, type)                  ((void)0)
>>  #  define DEBUG_GEN_VALUE(ctx, type, value)     ((void)0)
>>  #  define DEBUG_GEN_STRING(ctx, value, lenght)  ((void)0)
>> +#  define DEBUG_GEN_NUMBER(ctx, value, lenght)  ((void)0)
>
> that typo got propagated...
>
>>  #  define DEBUG_GEN_REPORT(ctx)                 ((void)0)
>>  #endif
>>
>> @@ -363,6 +365,7 @@ void libxl__json_object_free(libxl__gc *gc,
libxl__json_object *obj)
>>          return;
>>      switch (obj->type) {
>>      case JSON_STRING:
>> +    case JSON_NUMBER:
>>          free(obj->u.string);
>>          break;
>>      case JSON_MAP: {
>> @@ -504,35 +507,38 @@ static int json_callback_boolean(void *opaque,
int boolean)
>>      return 1;
>>  }
>>
>> -static int json_callback_integer(void *opaque, long value)
>> +static int json_callback_number(void *opaque, const char *s, unsigned
int len)
>>  {
>>      libxl__yajl_ctx *ctx = opaque;
>> -    libxl__json_object *obj;
>> -
>> -    DEBUG_GEN_VALUE(ctx, integer, value);
>> +    libxl__json_object *obj = NULL;
>> +    long long i;
>>
>> -    if ((obj = json_object_alloc(ctx->gc, JSON_INTEGER)) == NULL)
>> -        return 0;
>> -    obj->u.i = value;
>> +    /* should be replace by number */
>> +    DEBUG_GEN_NUMBER(ctx, s, len);
>>
>> -    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1)
{
>> -        libxl__json_object_free(ctx->gc, obj);
>> -        return 0;
>> -    }
>> +    i = strtoll(s, NULL, 10);
>>
>> -    return 1;
>> -}
>> +    if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE)
{
>> +        char *t = NULL;
>>
>> -static int json_callback_double(void *opaque, double value)
>> -{
>> -    libxl__yajl_ctx *ctx = opaque;
>> -    libxl__json_object *obj;
>> +        if ((obj = json_object_alloc(ctx->gc, JSON_NUMBER)) ==
NULL)
>> +            return 0;
>>
>> -    DEBUG_GEN_VALUE(ctx, double, value);
>> +        t = malloc(len + 1);
>> +        if (t == NULL) {
>> +            LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc),
LIBXL__LOG_ERROR,
>> +                             "Failed to allocate");
>> +            return 0;
>> +        }
>> +        strncpy(t, s, len);
>> +        t[len] = 0;
>>
>> -    if ((obj = json_object_alloc(ctx->gc, JSON_DOUBLE)) == NULL)
>> -        return 0;
>> -    obj->u.d = value;
>> +        obj->u.string = t;
>> +    } else {
>> +        if ((obj = json_object_alloc(ctx->gc, JSON_INTEGER)) ==
NULL)
>> +            return 0;
>> +        obj->u.i = i;
>> +    }
>>
>>      if (json_object_append_to(ctx->gc, obj, ctx->current) == -1)
{
>>          libxl__json_object_free(ctx->gc, obj);
>> @@ -706,9 +712,9 @@ static int json_callback_end_array(void *opaque)
>>  static yajl_callbacks callbacks = {
>>      json_callback_null,
>>      json_callback_boolean,
>> -    json_callback_integer,
>> -    json_callback_double,
>>      NULL,
>> +    NULL,
>> +    json_callback_number,
>>      json_callback_string,
>>      json_callback_start_map,
>>      json_callback_map_key,
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>


-- 
Anthony PERARD

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

On Mon, Oct 24, 2011 at 10:58, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
>> The all key is /libxl/$domid/dm-version.
>>
>> The /libxl/$domid dir is created with the domain and should be only
accessible
>> by the toolstack domain.
>>
>> This come with libxl__device_model_version_running helper function.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  tools/libxl/libxl.c          |    2 ++
>>  tools/libxl/libxl_create.c   |   28 ++++++++++++++++++++++++++++
>>  tools/libxl/libxl_internal.c |   19 +++++++++++++++++++
>>  tools/libxl/libxl_internal.h |    5 +++++
>>  4 files changed, 54 insertions(+), 0 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 064fbc4..50b97c2 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -777,6 +777,8 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t
domid, int force)
>>      if (!xs_rm(ctx->xsh, XBT_NULL, dom_path))
>>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs_rm failed for
%s", dom_path);
>>
>> +    xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(&gc,
"/libxl/%d", domid));
>> +
>>      libxl__userdata_destroyall(&gc, domid);
>>
>>      rc = xc_domain_destroy(ctx->xch, domid);
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 68d0fc3..bed991c 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -322,6 +322,9 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
>>      xs_transaction_t t = 0;
>>      xen_domain_handle_t handle;
>>
>> +    struct xs_permissions libxlperm[1];
>> +    char *libxl_path = NULL;
>> +
>
> The variable declaration block has little sections of perms and *_path
> -- you might as well include those there. libxlperm might be better
> called noperm (assuming that really is it''s meaning, xs''s
permission
> scheme confuses the hell out of me).
OK, I will change that.
>>      assert(!libxl_domid_valid_guest(*domid));
>>
>>      uuid_string = libxl__uuid2string(gc, info->uuid);
>> @@ -368,6 +371,15 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
>>          goto out;
>>      }
>>
>> +    libxl_path = libxl__sprintf(gc, "/libxl/%i", *domid);
>
> This is probably worthy of a little helper, similar to
> libxl__xs_get_dom_path(). (/vm/blah ought to have one too but
doesn''t).
I will do it.
>> +    if (!libxl_path) {
>> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create
paths");
>> +        rc = ERROR_FAIL;
>> +        goto out;
>> +    }
>> +    libxlperm[0].id = 0;
>> +    libxlperm[0].perms = XS_PERM_NONE;
>> +
>>      roperm[0].id = 0;
>>      roperm[0].perms = XS_PERM_NONE;
>>      roperm[1].id = *domid;
>> @@ -386,6 +398,10 @@ retry_transaction:
>>      xs_mkdir(ctx->xsh, t, vm_path);
>>      xs_set_permissions(ctx->xsh, t, vm_path, roperm,
ARRAY_SIZE(roperm));
>>
>> +    xs_rm(ctx->xsh, t, libxl_path);
>> +    xs_mkdir(ctx->xsh, t, libxl_path);
>> +    xs_set_permissions(ctx->xsh, t, libxl_path, libxlperm,
ARRAY_SIZE(libxlperm));
>> +
>>      xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm",
dom_path), vm_path, strlen(vm_path));
>>      rc = libxl__domain_rename(gc, *domid, 0, info->name, t);
>>      if (rc)
>> @@ -429,6 +445,16 @@ retry_transaction:
>>      return rc;
>>  }
>>
>> +static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
>> +                             libxl_device_model_info *dm_info)
>> +{
>> +    char *path = NULL;
>> +
>> +    path = libxl__sprintf(gc, "/libxl/%i/dm-version",
domid);
>> +    return libxl__xs_write(gc, XBT_NULL, path, libxl__strdup(gc,
>> +      
 libxl_device_model_version_to_string(dm_info->device_model_version)));
>> +}
>> +
>>  static int do_domain_create(libxl__gc *gc, libxl_domain_config
*d_config,
>>                              libxl_console_ready cb, void *priv,
>>                              uint32_t *domid_out, int restore_fd)
>> @@ -485,6 +511,8 @@ static int do_domain_create(libxl__gc *gc,
libxl_domain_config *d_config,
>>          goto error_out;
>>      }
>>
>> +    store_libxl_entry(gc, domid, dm_info);
>> +
>>      for (i = 0; i < d_config->num_disks; i++) {
>>          ret = libxl_device_disk_add(ctx, domid,
&d_config->disks[i]);
>>          if (ret) {
>> diff --git a/tools/libxl/libxl_internal.c
b/tools/libxl/libxl_internal.c
>> index 3993d8e..5d0a2d4 100644
>> --- a/tools/libxl/libxl_internal.c
>> +++ b/tools/libxl/libxl_internal.c
>> @@ -319,6 +319,25 @@ int libxl__fd_set_cloexec(int fd)
>>      return fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>>  }
>>
>> +libxl_device_model_version
libxl__device_model_version_running(libxl__gc *gc,
>> +                                                              
uint32_t domid)
>> +{
>> +    char *path = NULL;
>> +    char *dm_version = NULL;
>> +    libxl_device_model_version value;
>> +
>> +    path = libxl__sprintf(gc, "/libxl/%d/dm-version",
domid);
>> +    dm_version = libxl__xs_read(gc, XBT_NULL, path);
>> +    if (!dm_version) {
>> +        return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
>> +    }
>> +
>> +    if (libxl_device_model_version_from_string(dm_version, &value)
< 0) {
>
> I think this should be a fatal error, if you''ve managed to read
> something from this key and it isn''t one of the expect values then
> something bad has happened.
Yes, I will change this to a fatal error.
>> +        return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
>> +    }
>> +    return value;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/tools/libxl/libxl_internal.h
b/tools/libxl/libxl_internal.h
>> index 2e26ac6..5720b31 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -554,6 +554,11 @@ _hidden void libxl__json_object_free(libxl__gc
*gc, libxl__json_object *obj);
>>
>>  _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc, const
char *s);
>>
>> +  /* Based on /local/domain/$domid/dm-version xenstore key
>> +   * default is qemu xen traditional */
>> +_hidden libxl_device_model_version
>> +libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
>> +
>>  #endif
>>
>>  /*
-- 
Anthony PERARD

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

On Mon, Oct 24, 2011 at 11:01, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
>
>> +    rc = qmp_synchronous_send(qmp, "device_add", &args,
NULL, qmp->timeout);
>> +    if (rc == 0) {
>> +        rc = qmp_synchronous_send(qmp, "query-pci", NULL,
>> +                                  &request, qmp->timeout);
>> +        if (rc == 0) {
>> +            rc = request.rc;
>> +        }
>
> Is every caller going to want this behaviour? Perhaps it belongs in
> qmp_sync..._send?
Yes, I think we want to know if the command and the callback have been
a success or not. And, in case we want to know if it''s a protocol
error or an error in the callback, then we will just need different
value return.

-- 
Anthony PERARD

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

Anthony PERARD writes ("[Xen-devel] [PATCH V2 02/11] libxl_json, Check the
parse> -    status =
yajl_parse_complete(yajl_ctx.hand);
> +    if (status == yajl_status_ok) {
> +        status = yajl_parse_complete(yajl_ctx.hand);
> +    }
Perhaps this would be better done with the "goto out" pattern ?

Ie:

    status = yajl_do_something();
    if (status != yajl_status_ok) goto out;

    status = ....

    ....

    return o;

  out:
    blah blah get_error blah blah
    return NULL;

That would make the logic clearer I think.

Ian.

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

Anthony PERARD writes ("[Xen-devel] [PATCH V2 08/11] libxl_qmp, Introduce
qmp_request_handle."):
> This structure helps keep the return code of the callback, so a
> caller can read it.
This is heading in a plausible direction but:

 * You need to state which of the members of the struct are for use by
   which parts of libxl, and when any shared members are written etc.

 * "handle" is the wrong word for this.  "handle" would
refer to a
   pointer or perhaps descriptor.  This is a context/info structure.

 * The "free_it" parameter to qmp_send seems odd.  Surely it should
   just be the case that the same person that allocated the context
   should free it ?

Ian.

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