thr3ads.net - Xen devel - [PATCH V2 0/9] Set dirty log on qemu-xen. [Sep 2012]

If this information is useful, please help other people find it:
Share via:

Anthony PERARD

2012-Sep-17 18:22 UTC

[PATCH V2 0/9] Set dirty log on qemu-xen.

This patch series enables libxl to set dirty log on QEMU upstream during
migration through a new QMP command.

The success of the call depends on the presence of the specific QMP command
xen-set-global-dirty-log in QEMU. Patches for this command have been sent.

There is several patches that cleanup a bit the libxl_json/qmp codes.

Changes since v1:
  - Use libxl allocation function with NOGC when requiered.
  - No more check of failed allocation.
  - New qmp_run_command function, to factorize the libxl_qmp code.
  - cleanup ...


Anthony PERARD (9):
  libxl_json: Use libxl alloc function with NOGC.
  libxl_json: Export json_object related function.
  libxl_json: Introduce libxl__json_object_to_yajl_gen.
  libxl_qmp: Introduces helpers to create an argument list.
  libxl_qmp: Use qmp_parameters_* functions for param list of a QMP
    command.
  libxl_qmp: Simplify run of single QMP commands.
  libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.
  libxl_dom: Call the right switch logdirty for the right DM.
  libxl: Allow migration with qemu-xen.

 tools/libxl/libxl.c          |   4 -
 tools/libxl/libxl_dom.c      |  45 +++++++++-
 tools/libxl/libxl_internal.h |  13 +++
 tools/libxl/libxl_json.c     | 116 ++++++++++++++++--------
 tools/libxl/libxl_qmp.c      | 204 +++++++++++++++++++++++--------------------
 5 files changed, 247 insertions(+), 135 deletions(-)

-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 1/9] libxl_json: Use libxl alloc function with NOGC.

This patch makes use of the libxl allocation API and removes the check for
allocation failure.

Also we don''t use GC with a json_object because this structure use
flexarray
and the latter does not use the gc.

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

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index caa8312..9c3dca2 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -210,12 +210,7 @@ static libxl__json_object *json_object_alloc(libxl__gc *gc,
 {
     libxl__json_object *obj;
 
-    obj = calloc(1, sizeof (libxl__json_object));
-    if (obj == NULL) {
-        LIBXL__LOG_ERRNO(libxl__gc_owner(gc), LIBXL__LOG_ERROR,
-                         "Failed to allocate a libxl__json_object");
-        return NULL;
-    }
+    obj = libxl__zalloc(NOGC, sizeof(*obj));
 
     obj->type = type;
 
@@ -393,8 +388,7 @@ static int json_callback_null(void *opaque)
 
     DEBUG_GEN(ctx, null);
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_NULL)) == NULL)
-        return 0;
+    obj = json_object_alloc(ctx->gc, JSON_NULL);
 
     if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
         libxl__json_object_free(ctx->gc, obj);
@@ -411,9 +405,8 @@ static int json_callback_boolean(void *opaque, int boolean)
 
     DEBUG_GEN_VALUE(ctx, bool, boolean);
 
-    if ((obj = json_object_alloc(ctx->gc,
-                                 boolean ? JSON_TRUE : JSON_FALSE)) == NULL)
-        return 0;
+    obj = json_object_alloc(ctx->gc,
+                            boolean ? JSON_TRUE : JSON_FALSE);
 
     if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
         libxl__json_object_free(ctx->gc, obj);
@@ -448,8 +441,7 @@ static int json_callback_number(void *opaque, const char *s,
libxl_yajl_length l
             goto error;
         }
 
-        if ((obj = json_object_alloc(ctx->gc, JSON_DOUBLE)) == NULL)
-            return 0;
+        obj = json_object_alloc(ctx->gc, JSON_DOUBLE);
         obj->u.d = d;
     } else {
         long long i = strtoll(s, NULL, 10);
@@ -458,16 +450,14 @@ static int json_callback_number(void *opaque, const char
*s, libxl_yajl_length l
             goto error;
         }
 
-        if ((obj = json_object_alloc(ctx->gc, JSON_INTEGER)) == NULL)
-            return 0;
+        obj = json_object_alloc(ctx->gc, JSON_INTEGER);
         obj->u.i = i;
     }
     goto out;
 
 error:
     /* If the conversion fail, we just store the original string. */
-    if ((obj = json_object_alloc(ctx->gc, JSON_NUMBER)) == NULL)
-        return 0;
+    obj = json_object_alloc(ctx->gc, JSON_NUMBER);
 
     t = malloc(len + 1);
     if (t == NULL) {
@@ -508,10 +498,7 @@ static int json_callback_string(void *opaque, const
unsigned char *str,
     strncpy(t, (const char *) str, len);
     t[len] = 0;
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_STRING)) == NULL) {
-        free(t);
-        return 0;
-    }
+    obj = json_object_alloc(ctx->gc, JSON_STRING);
     obj->u.string = t;
 
     if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
@@ -573,8 +560,7 @@ static int json_callback_start_map(void *opaque)
 
     DEBUG_GEN(ctx, map_open);
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_MAP)) == NULL)
-        return 0;
+    obj = json_object_alloc(ctx->gc, JSON_MAP);
 
     if (ctx->current) {
         if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
@@ -615,8 +601,7 @@ static int json_callback_start_array(void *opaque)
 
     DEBUG_GEN(ctx, array_open);
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_ARRAY)) == NULL)
-        return 0;
+    obj = json_object_alloc(ctx->gc, JSON_ARRAY);
 
     if (ctx->current) {
         if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
@@ -681,7 +666,7 @@ libxl__json_object *libxl__json_parse(libxl__gc *gc, const
char *s)
     unsigned char *str = NULL;
 
     memset(&yajl_ctx, 0, sizeof (yajl_ctx));
-    yajl_ctx.gc = gc;
+    yajl_ctx.gc = NOGC;
 
     DEBUG_GEN_ALLOC(&yajl_ctx);
 
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 2/9] libxl_json: Export json_object related function.

Export libxl__json_object_alloc and libxl__json_object_append_to to use them in
a later patch.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b6f54ba..3c2dcaa 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1511,6 +1511,14 @@ static inline long long
libxl__json_object_get_integer(const libxl__json_object
         return -1;
 }
 
+/* Objects allocated using this function must be free with
+ * libxl__json_object_free.
+ */
+_hidden libxl__json_object *libxl__json_object_alloc(libxl__gc *gc,
+                                                     libxl__json_node_type
type);
+_hidden int libxl__json_object_append_to(libxl__gc *gc,
+                                         libxl__json_object *obj,
+                                         libxl__json_object *dst);
 _hidden libxl__json_object *libxl__json_array_get(const libxl__json_object *o,
                                                   int i);
 _hidden
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 9c3dca2..0539865 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -205,7 +205,7 @@ yajl_gen_status libxl__string_gen_json(yajl_gen hand,
  * libxl__json_object helper functions
  */
 
-static libxl__json_object *json_object_alloc(libxl__gc *gc,
+libxl__json_object *libxl__json_object_alloc(libxl__gc *gc,
                                              libxl__json_node_type type)
 {
     libxl__json_object *obj;
@@ -231,7 +231,7 @@ static libxl__json_object *json_object_alloc(libxl__gc *gc,
     return obj;
 }
 
-static int json_object_append_to(libxl__gc *gc,
+int libxl__json_object_append_to(libxl__gc *gc,
                                  libxl__json_object *obj,
                                  libxl__json_object *dst)
 {
@@ -388,9 +388,9 @@ static int json_callback_null(void *opaque)
 
     DEBUG_GEN(ctx, null);
 
-    obj = json_object_alloc(ctx->gc, JSON_NULL);
+    obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
 
-    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
         libxl__json_object_free(ctx->gc, obj);
         return 0;
     }
@@ -405,10 +405,10 @@ static int json_callback_boolean(void *opaque, int
boolean)
 
     DEBUG_GEN_VALUE(ctx, bool, boolean);
 
-    obj = json_object_alloc(ctx->gc,
-                            boolean ? JSON_TRUE : JSON_FALSE);
+    obj = libxl__json_object_alloc(ctx->gc,
+                                   boolean ? JSON_TRUE : JSON_FALSE);
 
-    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
         libxl__json_object_free(ctx->gc, obj);
         return 0;
     }
@@ -441,7 +441,7 @@ static int json_callback_number(void *opaque, const char *s,
libxl_yajl_length l
             goto error;
         }
 
-        obj = json_object_alloc(ctx->gc, JSON_DOUBLE);
+        obj = libxl__json_object_alloc(ctx->gc, JSON_DOUBLE);
         obj->u.d = d;
     } else {
         long long i = strtoll(s, NULL, 10);
@@ -450,14 +450,14 @@ static int json_callback_number(void *opaque, const char
*s, libxl_yajl_length l
             goto error;
         }
 
-        obj = json_object_alloc(ctx->gc, JSON_INTEGER);
+        obj = libxl__json_object_alloc(ctx->gc, JSON_INTEGER);
         obj->u.i = i;
     }
     goto out;
 
 error:
     /* If the conversion fail, we just store the original string. */
-    obj = json_object_alloc(ctx->gc, JSON_NUMBER);
+    obj = libxl__json_object_alloc(ctx->gc, JSON_NUMBER);
 
     t = malloc(len + 1);
     if (t == NULL) {
@@ -471,7 +471,7 @@ error:
     obj->u.string = t;
 
 out:
-    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
         libxl__json_object_free(ctx->gc, obj);
         return 0;
     }
@@ -498,10 +498,10 @@ static int json_callback_string(void *opaque, const
unsigned char *str,
     strncpy(t, (const char *) str, len);
     t[len] = 0;
 
-    obj = json_object_alloc(ctx->gc, JSON_STRING);
+    obj = libxl__json_object_alloc(ctx->gc, JSON_STRING);
     obj->u.string = t;
 
-    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
         libxl__json_object_free(ctx->gc, obj);
         return 0;
     }
@@ -560,10 +560,10 @@ static int json_callback_start_map(void *opaque)
 
     DEBUG_GEN(ctx, map_open);
 
-    obj = json_object_alloc(ctx->gc, JSON_MAP);
+    obj = libxl__json_object_alloc(ctx->gc, JSON_MAP);
 
     if (ctx->current) {
-        if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) ==
-1) {
             libxl__json_object_free(ctx->gc, obj);
             return 0;
         }
@@ -601,10 +601,10 @@ static int json_callback_start_array(void *opaque)
 
     DEBUG_GEN(ctx, array_open);
 
-    obj = json_object_alloc(ctx->gc, JSON_ARRAY);
+    obj = libxl__json_object_alloc(ctx->gc, JSON_ARRAY);
 
     if (ctx->current) {
-        if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) ==
-1) {
             libxl__json_object_free(ctx->gc, obj);
             return 0;
         }
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

This function converts a libxl__json_object to yajl by calling every yajl_gen_*
function on a preallocated yajl_gen hand.

This helps to integrate a json_object into an already existing yajl_gen tree.

This function is used in a later patch.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_json.c     | 63 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3c2dcaa..9c1482d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1527,6 +1527,9 @@ libxl__json_map_node *libxl__json_map_node_get(const
libxl__json_object *o,
 _hidden const libxl__json_object *libxl__json_map_get(const char *key,
                                           const libxl__json_object *o,
                                           libxl__json_node_type expected_type);
+_hidden yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
+                                                   yajl_gen hand,
+                                                   libxl__json_object *param);
 _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);
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 0539865..40818f2 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -376,6 +376,69 @@ const libxl__json_object *libxl__json_map_get(const char
*key,
     return NULL;
 }
 
+yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
+                                           yajl_gen hand,
+                                           libxl__json_object *obj)
+{
+    int index = 0;
+    yajl_status rc;
+
+    switch (obj->type) {
+    case JSON_NULL:
+        return yajl_gen_null(hand);
+    case JSON_TRUE:
+        return yajl_gen_bool(hand, true);
+    case JSON_FALSE:
+        return yajl_gen_bool(hand, false);
+    case JSON_INTEGER:
+        return yajl_gen_integer(hand, obj->u.i);
+    case JSON_DOUBLE:
+        return yajl_gen_double(hand, obj->u.d);
+    case JSON_NUMBER:
+        return yajl_gen_number(hand, obj->u.string,
strlen(obj->u.string));
+    case JSON_STRING:
+        return libxl__yajl_gen_asciiz(hand, obj->u.string);
+    case JSON_MAP: {
+        libxl__json_map_node *node = NULL;
+
+        rc = yajl_gen_map_open(hand);
+        if (rc != yajl_status_ok)
+            return rc;
+        for (index = 0; index < obj->u.map->count; index++) {
+            if (flexarray_get(obj->u.map, index, (void**)&node) != 0)
+                break;
+
+            rc = libxl__yajl_gen_asciiz(hand, node->map_key);
+            if (rc != yajl_status_ok)
+                return rc;
+            rc = libxl__json_object_to_yajl_gen(gc, hand, node->obj);
+            if (rc != yajl_status_ok)
+                return rc;
+        }
+        return yajl_gen_map_close(hand);
+    }
+    case JSON_ARRAY: {
+        libxl__json_object *node = NULL;
+
+        rc = yajl_gen_array_open(hand);
+        if (rc != yajl_status_ok)
+            return rc;
+        for (index = 0; index < obj->u.array->count; index++) {
+            if (flexarray_get(obj->u.array, index, (void**)&node) != 0)
+                break;
+            rc = libxl__json_object_to_yajl_gen(gc, hand, node);
+            if (rc != yajl_status_ok)
+                return rc;
+        }
+        return yajl_gen_array_close(hand);
+    }
+    case JSON_ERROR:
+    case JSON_ANY:
+    default:
+        return -1;
+    }
+}
+
 
 /*
  * JSON callbacks
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

Those functions will be used to create a "list" of parameters that
contain more
than just strings. This list is converted by qmp_send to a string to be sent to
QEMU.

Those functions will be used in the next two patches, so right now there are
not compiled.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 07a8bd5..1dd5c6c 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -623,6 +623,59 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
     free(qmp);
 }
 
+#if 0
+/*
+ * QMP Parameters Helpers
+ * Those functions does not use the gc, because of the internal usage of
+ * flexarray that does not support it.
+ * The allocated *param need to be free with libxl__json_object_free(gc, param)
+ */
+static void qmp_parameters_common_add(libxl__gc *gc,
+                                      libxl__json_object **param,
+                                      const char *name,
+                                      libxl__json_object *obj)
+{
+    libxl__json_map_node *arg = NULL;
+
+    if (!*param) {
+        *param = libxl__json_object_alloc(NOGC, JSON_MAP);
+    }
+
+    arg = libxl__zalloc(NOGC, sizeof(*arg));
+
+    arg->map_key = libxl__strdup(NOGC, name);
+    arg->obj = obj;
+
+    flexarray_append((*param)->u.map, arg);
+}
+
+static void qmp_parameters_add_string(libxl__gc *gc,
+                                      libxl__json_object **param,
+                                      const char *name, const char *argument)
+{
+    libxl__json_object *obj;
+
+    obj = libxl__json_object_alloc(NOGC, JSON_STRING);
+    obj->u.string = libxl__strdup(NOGC, argument);
+
+    qmp_parameters_common_add(gc, param, name, obj);
+}
+
+static void qmp_parameters_add_bool(libxl__gc *gc,
+                                    libxl__json_object **param,
+                                    const char *name, bool b)
+{
+    libxl__json_object *obj;
+
+    obj = libxl__json_object_alloc(NOGC, JSON_TRUE);
+    qmp_parameters_common_add(gc, param, name, obj);
+}
+
+#define QMP_PARAMETERS_SPRINTF(gc, args, name, format, ...) \
+    qmp_parameters_add_string(gc, args, name, \
+                              libxl__sprintf(gc, format, __VA_ARGS__))
+#endif
+
 /*
  * API
  */
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 5/9] libxl_qmp: Use qmp_parameters_* functions for param list of a QMP command.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 1dd5c6c..ed67431 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -78,7 +78,7 @@ struct libxl__qmp_handler {
 };
 
 static int qmp_send(libxl__qmp_handler *qmp,
-                    const char *cmd, libxl_key_value_list *args,
+                    const char *cmd, libxl__json_object *args,
                     qmp_callback_t callback, void *opaque,
                     qmp_request_context *context);
 
@@ -502,7 +502,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
 }
 
 static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
-                              const char *cmd, libxl_key_value_list *args,
+                              const char *cmd, libxl__json_object *args,
                               qmp_callback_t callback, void *opaque,
                               qmp_request_context *context)
 {
@@ -526,7 +526,7 @@ static char *qmp_send_prepare(libxl__gc *gc,
libxl__qmp_handler *qmp,
     yajl_gen_integer(hand, ++qmp->last_id_used);
     if (args) {
         libxl__yajl_gen_asciiz(hand, "arguments");
-        libxl_key_value_list_gen_json(hand, args);
+        libxl__json_object_to_yajl_gen(gc, hand, args);
     }
     yajl_gen_map_close(hand);
 
@@ -560,7 +560,7 @@ out:
 }
 
 static int qmp_send(libxl__qmp_handler *qmp,
-                    const char *cmd, libxl_key_value_list *args,
+                    const char *cmd, libxl__json_object *args,
                     qmp_callback_t callback, void *opaque,
                     qmp_request_context *context)
 {
@@ -588,7 +588,7 @@ out:
 }
 
 static int qmp_synchronous_send(libxl__qmp_handler *qmp, const char *cmd,
-                                libxl_key_value_list *args,
+                                libxl__json_object *args,
                                 qmp_callback_t callback, void *opaque,
                                 int ask_timeout)
 {
@@ -623,7 +623,6 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
     free(qmp);
 }
 
-#if 0
 /*
  * QMP Parameters Helpers
  * Those functions does not use the gc, because of the internal usage of
@@ -661,6 +660,7 @@ static void qmp_parameters_add_string(libxl__gc *gc,
     qmp_parameters_common_add(gc, param, name, obj);
 }
 
+#if 0
 static void qmp_parameters_add_bool(libxl__gc *gc,
                                     libxl__json_object **param,
                                     const char *name, bool b)
@@ -670,11 +670,11 @@ static void qmp_parameters_add_bool(libxl__gc *gc,
     obj = libxl__json_object_alloc(NOGC, JSON_TRUE);
     qmp_parameters_common_add(gc, param, name, obj);
 }
+#endif
 
 #define QMP_PARAMETERS_SPRINTF(gc, args, name, format, ...) \
     qmp_parameters_add_string(gc, args, name, \
                               libxl__sprintf(gc, format, __VA_ARGS__))
-#endif
 
 /*
  * API
@@ -802,8 +802,7 @@ out:
 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;
+    libxl__json_object *args = NULL;
     char *hostaddr = NULL;
     int rc = 0;
 
@@ -816,31 +815,23 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid,
libxl_device_pci *pcidev)
     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);
+    qmp_parameters_add_string(gc, &args, "driver",
"xen-pci-passthrough");
+    QMP_PARAMETERS_SPRINTF(gc, &args, "id", PCI_PT_QDEV_ID,
+                           pcidev->bus, pcidev->dev, pcidev->func);
+    qmp_parameters_add_string(gc, &args, "hostaddr", hostaddr);
     if (pcidev->vdevfn) {
-        flexarray_append_pair(parameters, "addr",
-                              libxl__sprintf(gc, "%x.%x",
-                                             PCI_SLOT(pcidev->vdevfn),
-                                             PCI_FUNC(pcidev->vdevfn)));
+        QMP_PARAMETERS_SPRINTF(gc, &args, "addr",
"%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,
+    rc = qmp_synchronous_send(qmp, "device_add", args,
                               NULL, NULL, qmp->timeout);
     if (rc == 0) {
         rc = qmp_synchronous_send(qmp, "query-pci", NULL,
                                   pci_add_callback, pcidev, qmp->timeout);
     }
 
-    flexarray_free(parameters);
+    libxl__json_object_free(gc, args);
     libxl__qmp_close(qmp);
     return rc;
 }
@@ -848,24 +839,19 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid,
libxl_device_pci *pcidev)
 static int qmp_device_del(libxl__gc *gc, int domid, char *id)
 {
     libxl__qmp_handler *qmp = NULL;
-    flexarray_t *parameters = NULL;
-    libxl_key_value_list args = NULL;
+    libxl__json_object *args = NULL;
     int rc = 0;
 
     qmp = libxl__qmp_initialize(gc, domid);
     if (!qmp)
         return ERROR_FAIL;
 
-    parameters = flexarray_make(2, 1);
-    flexarray_append_pair(parameters, "id", id);
-    args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
-    if (!args)
-        return ERROR_NOMEM;
+    qmp_parameters_add_string(gc, &args, "id", id);
 
-    rc = qmp_synchronous_send(qmp, "device_del", &args,
+    rc = qmp_synchronous_send(qmp, "device_del", args,
                               NULL, NULL, qmp->timeout);
+    libxl__json_object_free(gc, args);
 
-    flexarray_free(parameters);
     libxl__qmp_close(qmp);
     return rc;
 }
@@ -883,56 +869,38 @@ int libxl__qmp_pci_del(libxl__gc *gc, int domid,
libxl_device_pci *pcidev)
 int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename)
 {
     libxl__qmp_handler *qmp = NULL;
-    flexarray_t *parameters = NULL;
-    libxl_key_value_list args = NULL;
+    libxl__json_object *args = NULL;
     int rc = 0;
 
     qmp = libxl__qmp_initialize(gc, domid);
     if (!qmp)
         return ERROR_FAIL;
 
-    parameters = flexarray_make(2, 1);
-    if (!parameters) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
-    flexarray_append_pair(parameters, "filename", (char *)filename);
-    args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
-    if (!args) {
-        rc = ERROR_NOMEM;
-        goto out2;
-    }
+    qmp_parameters_add_string(gc, &args, "filename", (char
*)filename);
 
-    rc = qmp_synchronous_send(qmp, "xen-save-devices-state",
&args,
+    rc = qmp_synchronous_send(qmp, "xen-save-devices-state", args,
                               NULL, NULL, qmp->timeout);
+    libxl__json_object_free(gc, args);
 
-out2:
-    flexarray_free(parameters);
-out:
-    libxl__qmp_close(qmp);
     return rc;
 }
 
 static int qmp_change(libxl__gc *gc, libxl__qmp_handler *qmp,
                       char *device, char *target, char *arg)
 {
-    flexarray_t *parameters = NULL;
-    libxl_key_value_list args = NULL;
+    libxl__json_object *args = NULL;
     int rc = 0;
 
-    parameters = flexarray_make(6, 1);
-    flexarray_append_pair(parameters, "device", device);
-    flexarray_append_pair(parameters, "target", target);
-    if (arg)
-        flexarray_append_pair(parameters, "arg", arg);
-    args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
-    if (!args)
-        return ERROR_NOMEM;
+    qmp_parameters_add_string(gc, &args, "device", device);
+    qmp_parameters_add_string(gc, &args, "target", target);
+    if (arg) {
+        qmp_parameters_add_string(gc, &args, "arg", arg);
+    }
 
-    rc = qmp_synchronous_send(qmp, "change", &args,
+    rc = qmp_synchronous_send(qmp, "change", args,
                               NULL, NULL, qmp->timeout);
+    libxl__json_object_free(gc, args);
 
-    flexarray_free(parameters);
     return rc;
 }
 
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 6/9] libxl_qmp: Simplify run of single QMP commands.

This new function connects to QEMU, sends the command and disconnects.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index ed67431..aac06c2 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -799,6 +799,23 @@ out:
     return rc;
 }
 
+static int qmp_run_command(libxl__gc *gc, int domid,
+                           const char *cmd, libxl__json_object *args,
+                           qmp_callback_t callback, void *opaque)
+{
+    libxl__qmp_handler *qmp = NULL;
+    int rc = 0;
+
+    qmp = libxl__qmp_initialize(gc, domid);
+    if (!qmp)
+        return ERROR_FAIL;
+
+    rc = qmp_synchronous_send(qmp, cmd, args, callback, opaque,
qmp->timeout);
+
+    libxl__qmp_close(qmp);
+    return rc;
+}
+
 int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
 {
     libxl__qmp_handler *qmp = NULL;
@@ -838,21 +855,14 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid,
libxl_device_pci *pcidev)
 
 static int qmp_device_del(libxl__gc *gc, int domid, char *id)
 {
-    libxl__qmp_handler *qmp = NULL;
     libxl__json_object *args = NULL;
     int rc = 0;
 
-    qmp = libxl__qmp_initialize(gc, domid);
-    if (!qmp)
-        return ERROR_FAIL;
-
     qmp_parameters_add_string(gc, &args, "id", id);
 
-    rc = qmp_synchronous_send(qmp, "device_del", args,
-                              NULL, NULL, qmp->timeout);
+    rc = qmp_run_command(gc, domid, "device_del", args, NULL, NULL);
     libxl__json_object_free(gc, args);
 
-    libxl__qmp_close(qmp);
     return rc;
 }
 
@@ -868,18 +878,13 @@ int libxl__qmp_pci_del(libxl__gc *gc, int domid,
libxl_device_pci *pcidev)
 
 int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename)
 {
-    libxl__qmp_handler *qmp = NULL;
     libxl__json_object *args = NULL;
     int rc = 0;
 
-    qmp = libxl__qmp_initialize(gc, domid);
-    if (!qmp)
-        return ERROR_FAIL;
-
     qmp_parameters_add_string(gc, &args, "filename", (char
*)filename);
 
-    rc = qmp_synchronous_send(qmp, "xen-save-devices-state", args,
-                              NULL, NULL, qmp->timeout);
+    rc = qmp_run_command(gc, domid, "xen-save-devices-state", args,
+                         NULL, NULL);
     libxl__json_object_free(gc, args);
 
     return rc;
@@ -906,34 +911,12 @@ static int qmp_change(libxl__gc *gc, libxl__qmp_handler
*qmp,
 
 int libxl__qmp_stop(libxl__gc *gc, int domid)
 {
-    libxl__qmp_handler *qmp = NULL;
-    int rc = 0;
-
-    qmp = libxl__qmp_initialize(gc, domid);
-    if (!qmp)
-        return ERROR_FAIL;
-
-    rc = qmp_synchronous_send(qmp, "stop", NULL,
-                              NULL, NULL, qmp->timeout);
-
-    libxl__qmp_close(qmp);
-    return rc;
+    return qmp_run_command(gc, domid, "stop", NULL, NULL, NULL);
 }
 
 int libxl__qmp_resume(libxl__gc *gc, int domid)
 {
-    libxl__qmp_handler *qmp = NULL;
-    int rc = 0;
-
-    qmp = libxl__qmp_initialize(gc, domid);
-    if (!qmp)
-        return ERROR_FAIL;
-
-    rc = qmp_synchronous_send(qmp, "cont", NULL,
-                              NULL, NULL, qmp->timeout);
-
-    libxl__qmp_close(qmp);
-    return rc;
+    return qmp_run_command(gc, domid, "cont", NULL, NULL, NULL);
 }
 
 int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

This function will enable or disable the global dirty log on QEMU, used during
a migration.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9c1482d..acdeeb2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1399,6 +1399,8 @@ _hidden int libxl__qmp_stop(libxl__gc *gc, int domid);
 _hidden int libxl__qmp_resume(libxl__gc *gc, int domid);
 /* Save current QEMU state into fd. */
 _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename);
+/* Set dirty bitmap logging status */
+_hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool
enable);
 /* 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 aac06c2..befa11b 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -660,7 +660,6 @@ static void qmp_parameters_add_string(libxl__gc *gc,
     qmp_parameters_common_add(gc, param, name, obj);
 }
 
-#if 0
 static void qmp_parameters_add_bool(libxl__gc *gc,
                                     libxl__json_object **param,
                                     const char *name, bool b)
@@ -670,7 +669,6 @@ static void qmp_parameters_add_bool(libxl__gc *gc,
     obj = libxl__json_object_alloc(NOGC, JSON_TRUE);
     qmp_parameters_common_add(gc, param, name, obj);
 }
-#endif
 
 #define QMP_PARAMETERS_SPRINTF(gc, args, name, format, ...) \
     qmp_parameters_add_string(gc, args, name, \
@@ -919,6 +917,20 @@ int libxl__qmp_resume(libxl__gc *gc, int domid)
     return qmp_run_command(gc, domid, "cont", NULL, NULL, NULL);
 }
 
+int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable)
+{
+    libxl__json_object *args = NULL;
+    int rc = 0;
+
+    qmp_parameters_add_bool(gc, &args, "enable", enable);
+
+    rc = qmp_run_command(gc, domid, "xen-set-global-dirty-log", args,
+                         NULL, NULL);
+    libxl__json_object_free(gc, args);
+
+    return rc;
+}
+
 int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
                                const libxl_domain_config *guest_config)
 {
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 8/9] libxl_dom: Call the right switch logdirty for the right DM.

This patch dispatch the switch logdirty call depending on which device model
version is running.

The call to qemu-xen is synchrone, not like the one to qemu-xen-traditional.

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

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index e1de832..95da18e 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -685,10 +685,10 @@ static void logdirty_init(libxl__logdirty_switch *lds)
     libxl__ev_time_init(&lds->timeout);
 }
 
-void libxl__domain_suspend_common_switch_qemu_logdirty
-                               (int domid, unsigned enable, void *user)
+static void domain_suspend_switch_qemu_xen_traditional_logdirty
+                               (int domid, unsigned enable,
+                                libxl__save_helper_state *shs)
 {
-    libxl__save_helper_state *shs = user;
     libxl__egc *egc = shs->egc;
     libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
     libxl__logdirty_switch *lds = &dss->logdirty;
@@ -756,6 +756,45 @@ void libxl__domain_suspend_common_switch_qemu_logdirty
     switch_logdirty_done(egc,dss,-1);
 }
 
+static void domain_suspend_switch_qemu_xen_logdirty
+                               (int domid, unsigned enable,
+                                libxl__save_helper_state *shs)
+{
+    libxl__egc *egc = shs->egc;
+    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
+    STATE_AO_GC(dss->ao);
+    int rc;
+
+    rc = libxl__qmp_set_global_dirty_log(gc, domid, enable);
+    if (!rc) {
+        libxl__xc_domain_saverestore_async_callback_done(egc, shs, 0);
+    } else {
+        LOG(ERROR,"logdirty switch failed (rc=%d), aborting
suspend",rc);
+        libxl__xc_domain_saverestore_async_callback_done(egc, shs, -1);
+    }
+}
+
+void libxl__domain_suspend_common_switch_qemu_logdirty
+                               (int domid, unsigned enable, void *user)
+{
+    libxl__save_helper_state *shs = user;
+    libxl__egc *egc = shs->egc;
+    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
+    STATE_AO_GC(dss->ao);
+
+    switch (libxl__device_model_version_running(gc, domid)) {
+    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+        domain_suspend_switch_qemu_xen_traditional_logdirty(domid, enable,
shs);
+        break;
+    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+        domain_suspend_switch_qemu_xen_logdirty(domid, enable, shs);
+        break;
+    default:
+        LOG(ERROR,"logdirty switch failed"
+            ", no valid device model version found, aborting
suspend");
+        libxl__xc_domain_saverestore_async_callback_done(egc, shs, -1);
+    }
+}
 static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev,
                                     const struct timeval *requested_abs)
 {
-- 
Anthony PERARD

Anthony PERARD

2012-Sep-17 18:22 UTC

head link

[PATCH V2 9/9] libxl: Allow migration with qemu-xen.

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

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0857c48..f08adf3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -770,10 +770,6 @@ int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid,
int fd, int flags,
     if (type == LIBXL_DOMAIN_TYPE_HVM && flags &
LIBXL_SUSPEND_LIVE) {
         switch (libxl__device_model_version_running(gc, domid)) {
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            LOG(ERROR,
-                "cannot live migrate HVM domains with qemu-xen
device-model");
-            rc = ERROR_FAIL;
-            goto out_err;
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
             /* No problem */
             break;
-- 
Anthony PERARD

Ian Campbell

2012-Sep-25 08:39 UTC

head link

Re: [PATCH V2 1/9] libxl_json: Use libxl alloc function with NOGC.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> This patch makes use of the libxl allocation API and removes the check for
> allocation failure.
> 
> Also we don''t use GC with a json_object because this structure use
flexarray
> and the latter does not use the gc.
It''s not an uncommon pattern in libxl for the content of the flexarray
to be gc''d but the actual array itself to be explicitly freed, often
implicitly via flexarray_contents(), if that''s what you want.

If we wanted I don''t think there''s any reason we
couldn''t make the
flexarray take a gc and use it, that would probably make things simpler
here and elsewhere and reduce the manual memory management (unless you
actually want/need that for some other reason).
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_json.c | 37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index caa8312..9c3dca2 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -210,12 +210,7 @@ static libxl__json_object *json_object_alloc(libxl__gc
*gc,
>  {
>      libxl__json_object *obj;
>  
> -    obj = calloc(1, sizeof (libxl__json_object));
> -    if (obj == NULL) {
> -        LIBXL__LOG_ERRNO(libxl__gc_owner(gc), LIBXL__LOG_ERROR,
> -                         "Failed to allocate a
libxl__json_object");
> -        return NULL;
> -    }
> +    obj = libxl__zalloc(NOGC, sizeof(*obj));
So you now ignore the gc passed in, which in any case you have now
caused to always be NOGC? Seems a bit round-about to me, why not use the
gc parameter here?

Ian.

Ian Campbell

2012-Sep-25 08:40 UTC

head link

Re: [PATCH V2 2/9] libxl_json: Export json_object related function.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> Export libxl__json_object_alloc and libxl__json_object_append_to to use
them in
> a later patch.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |  8 ++++++++
>  tools/libxl/libxl_json.c     | 34 +++++++++++++++++-----------------
>  2 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index b6f54ba..3c2dcaa 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1511,6 +1511,14 @@ static inline long long
libxl__json_object_get_integer(const libxl__json_object
>          return -1;
>  }
>  
> +/* Objects allocated using this function must be free with
> + * libxl__json_object_free.
> + */
> +_hidden libxl__json_object *libxl__json_object_alloc(libxl__gc *gc,
> +                                                     libxl__json_node_type
type);
> +_hidden int libxl__json_object_append_to(libxl__gc *gc,
> +                                         libxl__json_object *obj,
> +                                         libxl__json_object *dst);
>  _hidden libxl__json_object *libxl__json_array_get(const libxl__json_object
*o,
>                                                    int i);
>  _hidden
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index 9c3dca2..0539865 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -205,7 +205,7 @@ yajl_gen_status libxl__string_gen_json(yajl_gen hand,
>   * libxl__json_object helper functions
>   */
>  
> -static libxl__json_object *json_object_alloc(libxl__gc *gc,
> +libxl__json_object *libxl__json_object_alloc(libxl__gc *gc,
>                                               libxl__json_node_type type)
>  {
>      libxl__json_object *obj;
> @@ -231,7 +231,7 @@ static libxl__json_object *json_object_alloc(libxl__gc
*gc,
>      return obj;
>  }
>  
> -static int json_object_append_to(libxl__gc *gc,
> +int libxl__json_object_append_to(libxl__gc *gc,
>                                   libxl__json_object *obj,
>                                   libxl__json_object *dst)
>  {
> @@ -388,9 +388,9 @@ static int json_callback_null(void *opaque)
>  
>      DEBUG_GEN(ctx, null);
>  
> -    obj = json_object_alloc(ctx->gc, JSON_NULL);
> +    obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
>  
> -    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> +    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) ==
-1) {
>          libxl__json_object_free(ctx->gc, obj);
>          return 0;
>      }
> @@ -405,10 +405,10 @@ static int json_callback_boolean(void *opaque, int
boolean)
>  
>      DEBUG_GEN_VALUE(ctx, bool, boolean);
>  
> -    obj = json_object_alloc(ctx->gc,
> -                            boolean ? JSON_TRUE : JSON_FALSE);
> +    obj = libxl__json_object_alloc(ctx->gc,
> +                                   boolean ? JSON_TRUE : JSON_FALSE);
>  
> -    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> +    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) ==
-1) {
>          libxl__json_object_free(ctx->gc, obj);
>          return 0;
>      }
> @@ -441,7 +441,7 @@ static int json_callback_number(void *opaque, const
char *s, libxl_yajl_length l
>              goto error;
>          }
>  
> -        obj = json_object_alloc(ctx->gc, JSON_DOUBLE);
> +        obj = libxl__json_object_alloc(ctx->gc, JSON_DOUBLE);
>          obj->u.d = d;
>      } else {
>          long long i = strtoll(s, NULL, 10);
> @@ -450,14 +450,14 @@ static int json_callback_number(void *opaque, const
char *s, libxl_yajl_length l
>              goto error;
>          }
>  
> -        obj = json_object_alloc(ctx->gc, JSON_INTEGER);
> +        obj = libxl__json_object_alloc(ctx->gc, JSON_INTEGER);
>          obj->u.i = i;
>      }
>      goto out;
>  
>  error:
>      /* If the conversion fail, we just store the original string. */
> -    obj = json_object_alloc(ctx->gc, JSON_NUMBER);
> +    obj = libxl__json_object_alloc(ctx->gc, JSON_NUMBER);
>  
>      t = malloc(len + 1);
>      if (t == NULL) {
> @@ -471,7 +471,7 @@ error:
>      obj->u.string = t;
>  
>  out:
> -    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> +    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) ==
-1) {
>          libxl__json_object_free(ctx->gc, obj);
>          return 0;
>      }
> @@ -498,10 +498,10 @@ static int json_callback_string(void *opaque, const
unsigned char *str,
>      strncpy(t, (const char *) str, len);
>      t[len] = 0;
>  
> -    obj = json_object_alloc(ctx->gc, JSON_STRING);
> +    obj = libxl__json_object_alloc(ctx->gc, JSON_STRING);
>      obj->u.string = t;
>  
> -    if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> +    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) ==
-1) {
>          libxl__json_object_free(ctx->gc, obj);
>          return 0;
>      }
> @@ -560,10 +560,10 @@ static int json_callback_start_map(void *opaque)
>  
>      DEBUG_GEN(ctx, map_open);
>  
> -    obj = json_object_alloc(ctx->gc, JSON_MAP);
> +    obj = libxl__json_object_alloc(ctx->gc, JSON_MAP);
>  
>      if (ctx->current) {
> -        if (json_object_append_to(ctx->gc, obj, ctx->current) == -1)
{
> +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current)
== -1) {
>              libxl__json_object_free(ctx->gc, obj);
>              return 0;
>          }
> @@ -601,10 +601,10 @@ static int json_callback_start_array(void *opaque)
>  
>      DEBUG_GEN(ctx, array_open);
>  
> -    obj = json_object_alloc(ctx->gc, JSON_ARRAY);
> +    obj = libxl__json_object_alloc(ctx->gc, JSON_ARRAY);
>  
>      if (ctx->current) {
> -        if (json_object_append_to(ctx->gc, obj, ctx->current) == -1)
{
> +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current)
== -1) {
>              libxl__json_object_free(ctx->gc, obj);
>              return 0;
>          }

Ian Campbell

2012-Sep-25 08:44 UTC

head link

Re: [PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> This function converts a libxl__json_object to yajl by calling every
yajl_gen_*
> function on a preallocated yajl_gen hand.
> 
> This helps to integrate a json_object into an already existing yajl_gen
tree.
> 
> This function is used in a later patch.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |  3 +++
>  tools/libxl/libxl_json.c     | 63
++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 3c2dcaa..9c1482d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> [...]
> +    }
> +    case JSON_ERROR:
I can''t see this being used anywhere, could it just be removed from the
enum?
> +    case JSON_ANY:
Is JSON_ANY sort of like a JSON "void *"? Is it ever valid to be
passed
one here, IOW does this represent a coding error (==abort()) or a run
time one (== log something)?
> +    default:
If you skip this default case then some compilers will warn (a runtime
error) if we forget to add a new JSON_FOO here.
> +        return -1;
> +    }
> +}
> +
>  
>  /*
>   * JSON callbacks

Ian Campbell

2012-Sep-25 08:54 UTC

head link

Re: [PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> Those functions will be used to create a "list" of parameters
that contain more
> than just strings. This list is converted by qmp_send to a string to be
sent to
> QEMU.
> 
> Those functions will be used in the next two patches, so right now there
are
> not compiled.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_qmp.c | 53
+++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 07a8bd5..1dd5c6c 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -623,6 +623,59 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
>      free(qmp);
>  }
>  
> +#if 0
> +/*
> + * QMP Parameters Helpers
> + * Those functions does not use the gc, because of the internal usage of
> + * flexarray that does not support it.
> + * The allocated *param need to be free with libxl__json_object_free(gc,
param)
> + */
> +static void qmp_parameters_common_add(libxl__gc *gc,
> +                                      libxl__json_object **param,
> +                                      const char *name,
> +                                      libxl__json_object *obj)
> +{
> +    libxl__json_map_node *arg = NULL;
> +
> +    if (!*param) {
> +        *param = libxl__json_object_alloc(NOGC, JSON_MAP);
> +    }
> +
> +    arg = libxl__zalloc(NOGC, sizeof(*arg));
> +
> +    arg->map_key = libxl__strdup(NOGC, name);
> +    arg->obj = obj;
> +
> +    flexarray_append((*param)->u.map, arg);
> +}
> +
> +static void qmp_parameters_add_string(libxl__gc *gc,
> +                                      libxl__json_object **param,
> +                                      const char *name, const char
*argument)
> +{
> +    libxl__json_object *obj;
> +
> +    obj = libxl__json_object_alloc(NOGC, JSON_STRING);
> +    obj->u.string = libxl__strdup(NOGC, argument);
> +
> +    qmp_parameters_common_add(gc, param, name, obj);
> +}
> +
> +static void qmp_parameters_add_bool(libxl__gc *gc,
> +                                    libxl__json_object **param,
> +                                    const char *name, bool b)
> +{
> +    libxl__json_object *obj;
> +
> +    obj = libxl__json_object_alloc(NOGC, JSON_TRUE);
This is pre-existing, but treating JSON_TRUE and JSON_FALSE as distinct
node types and not specific values of the (currently non-existent)
JSON_BOOL type is a bit odd.

I noticed it because you don''t use the b param here... 
> +    qmp_parameters_common_add(gc, param, name, obj);
> +}
> +
> +#define QMP_PARAMETERS_SPRINTF(gc, args, name, format, ...) \
> +    qmp_parameters_add_string(gc, args, name, \
> +                              libxl__sprintf(gc, format, __VA_ARGS__))
I think it would be valid, and in keeping with the similar GC_SPRINTFOO
macros, to make gc and perhaps args required in the calling environment
rather than passing them, if you wanted to.
> +#endif
> +
>  /*
>   * API
>   */

Ian Campbell

2012-Sep-25 08:56 UTC

head link

Re: [PATCH V2 5/9] libxl_qmp: Use qmp_parameters_* functions for param list of a QMP command.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian Campbell

2012-Sep-25 08:57 UTC

head link

Re: [PATCH V2 6/9] libxl_qmp: Simplify run of single QMP commands.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> This new function connects to QEMU, sends the command and disconnects.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian Campbell

2012-Sep-25 09:06 UTC

head link

Re: [PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> Those functions will be used to create a "list" of parameters
that contain more
> than just strings. This list is converted by qmp_send to a string to be
sent to
> QEMU.
> 
> Those functions will be used in the next two patches, so right now there
are
> not compiled.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_qmp.c | 53
+++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 07a8bd5..1dd5c6c 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -623,6 +623,59 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
>      free(qmp);
>  }
>  
> +#if 0
> +/*
> + * QMP Parameters Helpers
> + * Those functions does not use the gc, because of the internal usage of
> + * flexarray that does not support it.
> + * The allocated *param need to be free with libxl__json_object_free(gc,
param)
> + */
> +static void qmp_parameters_common_add(libxl__gc *gc,
> +                                      libxl__json_object **param,
> +                                      const char *name,
> +                                      libxl__json_object *obj)
> +{
> +    libxl__json_map_node *arg = NULL;
> +
> +    if (!*param) {
> +        *param = libxl__json_object_alloc(NOGC, JSON_MAP);
> +    }
Having now read the callers in later patches this looks a bit odd to me
since they do

	args = NULL

	qmp_param.._add_foo(gc, args,...)

	qmp_run_thing

	libxl__json_object_free(...args...)

which is unbalanced WRT allocation and free and I can easily imagine
people forgetting the free.

If you can''t arrange for these object''s to be gc''d
then I think having
an explicit alloc in the caller would be less liable to surprise people.

#define qmp_args_alloc(gc) libxl__json_object_alloc(NOGC, JSON_MAP)
#define qmp_args_free(..) libxl__json_object_free(...args...)

would work nicely IMHO.

Ian.

Ian Campbell

2012-Sep-25 09:06 UTC

head link

Re: [PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> This function will enable or disable the global dirty log on QEMU, used
during
> a migration.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |  2 ++
>  tools/libxl/libxl_qmp.c      | 16 ++++++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 9c1482d..acdeeb2 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1399,6 +1399,8 @@ _hidden int libxl__qmp_stop(libxl__gc *gc, int
domid);
>  _hidden int libxl__qmp_resume(libxl__gc *gc, int domid);
>  /* Save current QEMU state into fd. */
>  _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char
*filename);
> +/* Set dirty bitmap logging status */
> +_hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool
enable);
>  /* 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 aac06c2..befa11b 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -660,7 +660,6 @@ static void qmp_parameters_add_string(libxl__gc *gc,
>      qmp_parameters_common_add(gc, param, name, obj);
>  }
>  
> -#if 0
>  static void qmp_parameters_add_bool(libxl__gc *gc,
>                                      libxl__json_object **param,
>                                      const char *name, bool b)
> @@ -670,7 +669,6 @@ static void qmp_parameters_add_bool(libxl__gc *gc,
>      obj = libxl__json_object_alloc(NOGC, JSON_TRUE);
>      qmp_parameters_common_add(gc, param, name, obj);
>  }
> -#endif
>  
>  #define QMP_PARAMETERS_SPRINTF(gc, args, name, format, ...) \
>      qmp_parameters_add_string(gc, args, name, \
> @@ -919,6 +917,20 @@ int libxl__qmp_resume(libxl__gc *gc, int domid)
>      return qmp_run_command(gc, domid, "cont", NULL, NULL, NULL);
>  }
>  
> +int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable)
> +{
> +    libxl__json_object *args = NULL;
> +    int rc = 0;
> +
> +    qmp_parameters_add_bool(gc, &args, "enable", enable);
> +
> +    rc = qmp_run_command(gc, domid, "xen-set-global-dirty-log",
args,
> +                         NULL, NULL);
> +    libxl__json_object_free(gc, args);
> +
> +    return rc;
> +}
> +
>  int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
>                                 const libxl_domain_config *guest_config)
>  {

Ian Campbell

2012-Sep-25 09:10 UTC

head link

Re: [PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

On Tue, 2012-09-25 at 10:06 +0100, Ian Campbell wrote:> > +    rc = qmp_run_command(gc, domid,
"xen-set-global-dirty-log", args,
> > +                         NULL, NULL);
For got to ask: Is this new function upstream and in our branch already?

I suppose there is no ordering constraint since we fail now, and if we
take this patch we''ll just fail more explicitly with an ENOSYS type
error unless/until the qemu half is in place.

Ian.

Ian Campbell

2012-Sep-25 09:22 UTC

head link

Re: [PATCH V2 8/9] libxl_dom: Call the right switch logdirty for the right DM.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> This patch dispatch the switch logdirty call depending on which device
model
> version is running.
> 
> The call to qemu-xen is synchrone, not like the one to
qemu-xen-traditional.
                          synchronous

IIRC there was talk of making the qmp calls asynchronous in the future,
since they might potentially be long running?

I also have some concerns about the existing use of the 3rd parameter to
switch_logdirty_done. In the prototype it takes an "int ok" but in the
definition takes a "int broke", which seems to be inverted. This value
is passed as the return_value argument to
libxl__xc_domain_saverestore_async_callback_done. The current callers
pass 0 (success) or -1 (error) and you follow that precedent, so you
patch is fine IMHO. But I wonder if there isn''t scope for some cleanup
here somewhere.

However I don''t think any of that should block this patch so:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

IanJ, while I was looking I noticed that remus_checkpoint_dm_saved calls
libxl__xc_domain_saverestore_async_callback_done directly instead of via
switch_logdirty_done like everyone else. Since the s_l_d cancels timers
and stuff too I wonder if this is a bug?

Ian.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_dom.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index e1de832..95da18e 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -685,10 +685,10 @@ static void logdirty_init(libxl__logdirty_switch
*lds)
>      libxl__ev_time_init(&lds->timeout);
>  }
>  
> -void libxl__domain_suspend_common_switch_qemu_logdirty
> -                               (int domid, unsigned enable, void *user)
> +static void domain_suspend_switch_qemu_xen_traditional_logdirty
> +                               (int domid, unsigned enable,
> +                                libxl__save_helper_state *shs)
>  {
> -    libxl__save_helper_state *shs = user;
>      libxl__egc *egc = shs->egc;
>      libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
>      libxl__logdirty_switch *lds = &dss->logdirty;
> @@ -756,6 +756,45 @@ void libxl__domain_suspend_common_switch_qemu_logdirty
>      switch_logdirty_done(egc,dss,-1);
>  }
>  
> +static void domain_suspend_switch_qemu_xen_logdirty
> +                               (int domid, unsigned enable,
> +                                libxl__save_helper_state *shs)
> +{
> +    libxl__egc *egc = shs->egc;
> +    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
> +    STATE_AO_GC(dss->ao);
> +    int rc;
> +
> +    rc = libxl__qmp_set_global_dirty_log(gc, domid, enable);
> +    if (!rc) {
> +        libxl__xc_domain_saverestore_async_callback_done(egc, shs, 0);
> +    } else {
> +        LOG(ERROR,"logdirty switch failed (rc=%d), aborting
suspend",rc);
> +        libxl__xc_domain_saverestore_async_callback_done(egc, shs, -1);
> +    }
> +}
> +
> +void libxl__domain_suspend_common_switch_qemu_logdirty
> +                               (int domid, unsigned enable, void *user)
> +{
> +    libxl__save_helper_state *shs = user;
> +    libxl__egc *egc = shs->egc;
> +    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
> +    STATE_AO_GC(dss->ao);
> +
> +    switch (libxl__device_model_version_running(gc, domid)) {
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +        domain_suspend_switch_qemu_xen_traditional_logdirty(domid, enable,
shs);
> +        break;
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +        domain_suspend_switch_qemu_xen_logdirty(domid, enable, shs);
> +        break;
> +    default:
> +        LOG(ERROR,"logdirty switch failed"
> +            ", no valid device model version found, aborting
suspend");
> +        libxl__xc_domain_saverestore_async_callback_done(egc, shs, -1);
> +    }
> +}
>  static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev,
>                                      const struct timeval *requested_abs)
>  {

Ian Campbell

2012-Sep-25 09:23 UTC

head link

Re: [PATCH V2 9/9] libxl: Allow migration with qemu-xen.

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
As far as I''m concerned you can just remove the entire check now, its
only purpose was to catch this case.

Ian.
> ---
>  tools/libxl/libxl.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 0857c48..f08adf3 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -770,10 +770,6 @@ int libxl_domain_suspend(libxl_ctx *ctx, uint32_t
domid, int fd, int flags,
>      if (type == LIBXL_DOMAIN_TYPE_HVM && flags &
LIBXL_SUSPEND_LIVE) {
>          switch (libxl__device_model_version_running(gc, domid)) {
>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> -            LOG(ERROR,
> -                "cannot live migrate HVM domains with qemu-xen
device-model");
> -            rc = ERROR_FAIL;
> -            goto out_err;
>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
>              /* No problem */
>              break;

Anthony PERARD

2012-Sep-25 13:54 UTC

head link

Re: [PATCH V2 1/9] libxl_json: Use libxl alloc function with NOGC.

On 09/25/2012 09:39 AM, Ian Campbell wrote:> On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:
>> This patch makes use of the libxl allocation API and removes the check
for
>> allocation failure.
>>
>> Also we don''t use GC with a json_object because this structure
use flexarray
>> and the latter does not use the gc.
> 
> It''s not an uncommon pattern in libxl for the content of the
flexarray
> to be gc''d but the actual array itself to be explicitly freed,
often
> implicitly via flexarray_contents(), if that''s what you want.
Trying to use flexarray_contents() in the context of json_object would
mean that I need to know when the array is filled with every things
needed. This seams a bit complicated.
> If we wanted I don''t think there''s any reason we
couldn''t make the
> flexarray take a gc and use it, that would probably make things simpler
> here and elsewhere and reduce the manual memory management (unless you
> actually want/need that for some other reason).
Having flexarray using gc seams a better idea. I will work on that as I
don''t think I need to keep those object around and using NOGC was only
because of flexarray.
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  tools/libxl/libxl_json.c | 37 +++++++++++--------------------------
>>  1 file changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
>> index caa8312..9c3dca2 100644
>> --- a/tools/libxl/libxl_json.c
>> +++ b/tools/libxl/libxl_json.c
>> @@ -210,12 +210,7 @@ static libxl__json_object
*json_object_alloc(libxl__gc *gc,
>>  {
>>      libxl__json_object *obj;
>>  
>> -    obj = calloc(1, sizeof (libxl__json_object));
>> -    if (obj == NULL) {
>> -        LIBXL__LOG_ERRNO(libxl__gc_owner(gc), LIBXL__LOG_ERROR,
>> -                         "Failed to allocate a
libxl__json_object");
>> -        return NULL;
>> -    }
>> +    obj = libxl__zalloc(NOGC, sizeof(*obj));
> 
> So you now ignore the gc passed in, which in any case you have now
> caused to always be NOGC? Seems a bit round-about to me, why not use the
> gc parameter here?
Yes, I suppose is not necessary to use NOGC here, and leave the choice
to the caller. I just wanted to be explicit.

Thanks,

-- 
Anthony PERARD

Anthony PERARD

2012-Sep-25 14:20 UTC

head link

Re: [PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

On 09/25/2012 09:44 AM, Ian Campbell wrote:> On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:
>> This function converts a libxl__json_object to yajl by calling every
yajl_gen_*
>> function on a preallocated yajl_gen hand.
>>
>> This helps to integrate a json_object into an already existing yajl_gen
tree.
>>
>> This function is used in a later patch.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  tools/libxl/libxl_internal.h |  3 +++
>>  tools/libxl/libxl_json.c     | 63
++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 66 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_internal.h
b/tools/libxl/libxl_internal.h
>> index 3c2dcaa..9c1482d 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> [...]
> 
>> +    }
>> +    case JSON_ERROR:
> 
> I can''t see this being used anywhere, could it just be removed
from the
> enum?
Yes.
>> +    case JSON_ANY:
> 
> Is JSON_ANY sort of like a JSON "void *"? Is it ever valid to be
passed
> one here, IOW does this represent a coding error (==abort()) or a run
> time one (== log something)?
This value is only used as parameter of one function,
libxl__json_map_get, that return an object from the json_object tree
with the expected type. But sometime we don''t expect any type so
JSON_ANY is used and allow "map_get" to return any type of object.

So JSON_ANY is not valid here and would probably be a coding error.
>> +    default:
> 
> If you skip this default case then some compilers will warn (a runtime
> error) if we forget to add a new JSON_FOO here.
I tried to remove the default, but then gcc believe that the function
would not return in some cases. But I did not add a return because it
felt weird to add a useless/never_executed return/abort.

But it seams better to remove the default case, so I will.
>> +        return -1;
>> +    }
>> +}
Thanks,

-- 
Anthony PERARD

Ian Campbell

2012-Sep-25 14:32 UTC

head link

Re: [PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

On Tue, 2012-09-25 at 15:20 +0100, Anthony PERARD wrote:> >> +    case JSON_ANY:
> > 
> > Is JSON_ANY sort of like a JSON "void *"? Is it ever valid
to be passed
> > one here, IOW does this represent a coding error (==abort()) or a run
> > time one (== log something)?
> 
> This value is only used as parameter of one function,
> libxl__json_map_get, that return an object from the json_object tree
> with the expected type. But sometime we don''t expect any type so
> JSON_ANY is used and allow "map_get" to return any type of
object.
> 
> So JSON_ANY is not valid here and would probably be a coding error.
Lets abort() then.
> 
> >> +    default:
> > 
> > If you skip this default case then some compilers will warn (a runtime
> > error) if we forget to add a new JSON_FOO here.
> 
> I tried to remove the default, but then gcc believe that the function
> would not return in some cases.
Gah! 
>  But I did not add a return because it
> felt weird to add a useless/never_executed return/abort.
> 
> But it seams better to remove the default case, so I will.
>

Anthony PERARD

2012-Sep-25 14:45 UTC

head link

Re: [PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

On 09/25/2012 09:54 AM, Ian Campbell wrote:> On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:
>> Those functions will be used to create a "list" of parameters
that contain more
>> than just strings. This list is converted by qmp_send to a string to be
sent to
>> QEMU.
>>
>> Those functions will be used in the next two patches, so right now
there are
>> not compiled.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  tools/libxl/libxl_qmp.c | 53
+++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> index 07a8bd5..1dd5c6c 100644
>> --- a/tools/libxl/libxl_qmp.c
>> +++ b/tools/libxl/libxl_qmp.c
>> @@ -623,6 +623,59 @@ static void qmp_free_handler(libxl__qmp_handler
*qmp)
>>      free(qmp);
>>  }
>>  
>> +#if 0
>> +/*
>> + * QMP Parameters Helpers
>> + * Those functions does not use the gc, because of the internal usage
of
>> + * flexarray that does not support it.
>> + * The allocated *param need to be free with
libxl__json_object_free(gc, param)
>> + */
>> +static void qmp_parameters_common_add(libxl__gc *gc,
>> +                                      libxl__json_object **param,
>> +                                      const char *name,
>> +                                      libxl__json_object *obj)
>> +{
>> +    libxl__json_map_node *arg = NULL;
>> +
>> +    if (!*param) {
>> +        *param = libxl__json_object_alloc(NOGC, JSON_MAP);
>> +    }
>> +
>> +    arg = libxl__zalloc(NOGC, sizeof(*arg));
>> +
>> +    arg->map_key = libxl__strdup(NOGC, name);
>> +    arg->obj = obj;
>> +
>> +    flexarray_append((*param)->u.map, arg);
>> +}
>> +
>> +static void qmp_parameters_add_string(libxl__gc *gc,
>> +                                      libxl__json_object **param,
>> +                                      const char *name, const char
*argument)
>> +{
>> +    libxl__json_object *obj;
>> +
>> +    obj = libxl__json_object_alloc(NOGC, JSON_STRING);
>> +    obj->u.string = libxl__strdup(NOGC, argument);
>> +
>> +    qmp_parameters_common_add(gc, param, name, obj);
>> +}
>> +
>> +static void qmp_parameters_add_bool(libxl__gc *gc,
>> +                                    libxl__json_object **param,
>> +                                    const char *name, bool b)
>> +{
>> +    libxl__json_object *obj;
>> +
>> +    obj = libxl__json_object_alloc(NOGC, JSON_TRUE);
> 
> This is pre-existing, but treating JSON_TRUE and JSON_FALSE as distinct
> node types and not specific values of the (currently non-existent)
> JSON_BOOL type is a bit odd.
It was to save a bit of memory, and I probably follow another example
when I wrote those JSON_{TRUE,FALSE}.

I''ll add another patch to introduce JSON_BOOL and remove those
separated
true/false.
> I noticed it because you don''t use the b param here... 
:(.
>> +    qmp_parameters_common_add(gc, param, name, obj);
>> +}
>> +
>> +#define QMP_PARAMETERS_SPRINTF(gc, args, name, format, ...) \
>> +    qmp_parameters_add_string(gc, args, name, \
>> +                              libxl__sprintf(gc, format, __VA_ARGS__))
> 
> I think it would be valid, and in keeping with the similar GC_SPRINTFOO
> macros, to make gc and perhaps args required in the calling environment
> rather than passing them, if you wanted to.
Seams fair to have gc required in the caller but I think I''ll keep args
as a parameter of the macro.

Thanks,

-- 
Anthony PERARD

Anthony PERARD

2012-Sep-25 14:51 UTC

head link

Re: [PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

On 09/25/2012 10:10 AM, Ian Campbell wrote:> On Tue, 2012-09-25 at 10:06 +0100, Ian Campbell wrote:
>>> +    rc = qmp_run_command(gc, domid,
"xen-set-global-dirty-log", args,
>>> +                         NULL, NULL);
> 
> For got to ask: Is this new function upstream and in our branch already?
No, the patches haven''t been accepted yet.
> I suppose there is no ordering constraint since we fail now, and if we
> take this patch we''ll just fail more explicitly with an ENOSYS
type
> error unless/until the qemu half is in place.
Yep, patches can be apply in any order. If QEMU does not know the
command, then it will return an error, and it will be relayed by libxl.


-- 
Anthony PERARD

Anthony PERARD

2012-Sep-25 14:53 UTC

head link

Re: [PATCH V2 9/9] libxl: Allow migration with qemu-xen.

On 09/25/2012 10:23 AM, Ian Campbell wrote:> On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> As far as I''m concerned you can just remove the entire check now,
its
> only purpose was to catch this case.
Will do.

-- 
Anthony PERARD

Xen devel - Sep 2012 - [PATCH V2 0/9] Set dirty log on qemu-xen.

[PATCH V2 0/9] Set dirty log on qemu-xen.

[PATCH V2 1/9] libxl_json: Use libxl alloc function with NOGC.

[PATCH V2 2/9] libxl_json: Export json_object related function.

[PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

[PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

[PATCH V2 5/9] libxl_qmp: Use qmp_parameters_* functions for param list of a QMP command.

[PATCH V2 6/9] libxl_qmp: Simplify run of single QMP commands.

[PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

[PATCH V2 8/9] libxl_dom: Call the right switch logdirty for the right DM.

[PATCH V2 9/9] libxl: Allow migration with qemu-xen.

Re: [PATCH V2 1/9] libxl_json: Use libxl alloc function with NOGC.

Re: [PATCH V2 2/9] libxl_json: Export json_object related function.

Re: [PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

Re: [PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

Re: [PATCH V2 5/9] libxl_qmp: Use qmp_parameters_* functions for param list of a QMP command.

Re: [PATCH V2 6/9] libxl_qmp: Simplify run of single QMP commands.

Re: [PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

Re: [PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

Re: [PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

Re: [PATCH V2 8/9] libxl_dom: Call the right switch logdirty for the right DM.

Re: [PATCH V2 9/9] libxl: Allow migration with qemu-xen.

Re: [PATCH V2 1/9] libxl_json: Use libxl alloc function with NOGC.

Re: [PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

Re: [PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

Re: [PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

Re: [PATCH V2 7/9] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

Re: [PATCH V2 9/9] libxl: Allow migration with qemu-xen.