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

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

[Anthony PERARD]

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.

Also, a recent patch to libxl will probably prevent this from working.


Anthony PERARD (6):
  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: Introduce libxl__qmp_set_global_dirty_log.
  libxl_dom: Call the right switch logdirty for the right DM.

 tools/libxl/libxl_dom.c      |   41 ++++++++-
 tools/libxl/libxl_internal.h |   10 ++
 tools/libxl/libxl_json.c     |   97 ++++++++++++++++---
 tools/libxl/libxl_qmp.c      |  207 ++++++++++++++++++++++++++++++++----------
 4 files changed, 289 insertions(+), 66 deletions(-)

-- 
Anthony PERARD

[PATCH 1/6] libxl_json: Export json_object related function.

[Anthony PERARD]

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 |    5 +++++
 tools/libxl/libxl_json.c     |   32 ++++++++++++++++----------------
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f832fbd..c233416 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1465,6 +1465,11 @@ static inline long long libxl__json_object_get_integer(const libxl__json_object
         return -1;
 }
 
+_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 e870606..9e302d5 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;
@@ -236,7 +236,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)
 {
@@ -393,10 +393,10 @@ static int json_callback_null(void *opaque)
 
     DEBUG_GEN(ctx, null);
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_NULL)) == NULL)
+    if ((obj = libxl__json_object_alloc(ctx->gc, JSON_NULL)) == NULL)
         return 0;
 
-    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;
     }
@@ -411,11 +411,11 @@ static int json_callback_boolean(void *opaque, int boolean)
 
     DEBUG_GEN_VALUE(ctx, bool, boolean);
 
-    if ((obj = json_object_alloc(ctx->gc,
+    if ((obj = libxl__json_object_alloc(ctx->gc,
                                  boolean ? JSON_TRUE : JSON_FALSE)) == NULL)
         return 0;
 
-    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;
     }
@@ -448,7 +448,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)
+        if ((obj = libxl__json_object_alloc(ctx->gc, JSON_DOUBLE)) == NULL)
             return 0;
         obj->u.d = d;
     } else {
@@ -458,7 +458,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_INTEGER)) == NULL)
+        if ((obj = libxl__json_object_alloc(ctx->gc, JSON_INTEGER)) == NULL)
             return 0;
         obj->u.i = i;
     }
@@ -466,7 +466,7 @@ static int json_callback_number(void *opaque, const char *s, libxl_yajl_length l
 
 error:
     /* If the conversion fail, we just store the original string. */
-    if ((obj = json_object_alloc(ctx->gc, JSON_NUMBER)) == NULL)
+    if ((obj = libxl__json_object_alloc(ctx->gc, JSON_NUMBER)) == NULL)
         return 0;
 
     t = malloc(len + 1);
@@ -481,7 +481,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;
     }
@@ -508,13 +508,13 @@ 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) {
+    if ((obj = libxl__json_object_alloc(ctx->gc, JSON_STRING)) == NULL) {
         free(t);
         return 0;
     }
     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;
     }
@@ -573,11 +573,11 @@ static int json_callback_start_map(void *opaque)
 
     DEBUG_GEN(ctx, map_open);
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_MAP)) == NULL)
+    if ((obj = libxl__json_object_alloc(ctx->gc, JSON_MAP)) == NULL)
         return 0;
 
     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;
         }
@@ -615,11 +615,11 @@ static int json_callback_start_array(void *opaque)
 
     DEBUG_GEN(ctx, array_open);
 
-    if ((obj = json_object_alloc(ctx->gc, JSON_ARRAY)) == NULL)
+    if ((obj = libxl__json_object_alloc(ctx->gc, JSON_ARRAY)) == NULL)
         return 0;
 
     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

Re: [PATCH 1/6] libxl_json: Export json_object related function.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 1/6] 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.

Now that we have decided that allocation failures will be fatal, this
function should perhaps be fixed not to use the libxl allocation
helpers with NOGC, and its call sites changed to assume success ?

Ian.

[PATCH 2/6] libxl_json: Introduce libxl__json_object_to_yajl_gen.

[Anthony PERARD]

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     |   65 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c233416..2176778 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1478,6 +1478,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 9e302d5..a93d3ce 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -381,6 +381,71 @@ 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;
+
+    if (obj == NULL)
+        return -1;
+    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

Re: [PATCH 2/6] libxl_json: Introduce libxl__json_object_to_yajl_gen.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 2/6] 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.

I'm slightly concerned at the amount of boilerplateish code we're
introducing here.  Forgive my utter ignorance but is there not some
more automatic way of handling all this ?

At least it looks reasonably generic so if there is no better way to
do it then fine.

I have some more detailed comments:

...
> +yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
> +                                           yajl_gen hand,
> +                                           libxl__json_object *obj)
> +{
> +    int index = 0;
> +    yajl_status rc;
> +
> +    if (obj == NULL)
> +        return -1;

Is this an expected situation ?  Surely we should crash if we pass
NULL rather than silently inventing an error code ?

> +    default:
> +        return -1;

When might this occur ?

Ian.

[PATCH 3/6] libxl_qmp: Introduces helpers to create an argument list.

[Anthony PERARD]

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 |   86 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index e33b130..edbd3b4 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -623,6 +623,92 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
     free(qmp);
 }
 
+#if 0
+/*
+ * QMP Parameters Helpers
+ * If a function fail to allocate memomy for one more parameters, then
+ * the param is free, even if it's come from a caller.
+ */
+static libxl__json_object *qmp_parameters_common(libxl__gc *gc,
+                                                 libxl__json_object *param,
+                                                 const char *name,
+                                                 libxl__json_object *obj)
+{
+    // check that every caller of this free the tree !
+    libxl__json_map_node *arg = NULL;
+    if (!param) {
+        param = libxl__json_object_alloc(gc, JSON_MAP);
+        if (!param)
+            return NULL;
+    }
+    arg = calloc(1, sizeof (libxl__json_map_node));
+    if (!arg)
+        goto error_nomem;
+
+    if (flexarray_append(param->u.map, arg) == 2) {
+        free(arg);
+        goto error_nomem;
+    }
+
+    arg->map_key = strdup(name);
+    if (!arg->map_key)
+        goto error_nomem;
+
+    arg->obj = obj;
+
+    return param;
+error_nomem:
+    libxl__json_object_free(gc, param);
+    return NULL;
+}
+
+static libxl__json_object *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(gc, JSON_STRING);
+    if (!obj)
+        goto error_nomem;
+    obj->u.string = strdup(argument);
+    if (!obj->u.string)
+        goto error_nomem;
+
+    param = qmp_parameters_common(gc, param, name, obj);
+    if (!param)
+        goto error_nomem;
+
+    return param;
+error_nomem:
+    libxl__json_object_free(gc, param);
+    libxl__json_object_free(gc, obj);
+    return NULL;
+}
+
+static libxl__json_object *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(gc, JSON_TRUE);
+    if (!obj)
+        goto error_nomem;
+
+    param = qmp_parameters_common(gc, param, name, obj);
+    if (!param)
+        goto error_nomem;
+
+    return param;
+error_nomem:
+    libxl__json_object_free(gc, param);
+    libxl__json_object_free(gc, obj);
+    return NULL;
+}
+#endif
+
 /*
  * API
  */
-- 
Anthony PERARD

Re: [PATCH 3/6] libxl_qmp: Introduces helpers to create an argument list.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 3/6] libxl_qmp: Introduces helpers to create an argument list."):
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index e33b130..edbd3b4 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -623,6 +623,92 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
>      free(qmp);
>  }
>  
> +#if 0
> +/*
> + * QMP Parameters Helpers
> + * If a function fail to allocate memomy for one more parameters, then
> + * the param is free, even if it's come from a caller.

Allocation failures are supposed to be fatal nowadays, so this case
doesn't need to exit.

> +static libxl__json_object *qmp_parameters_common(libxl__gc *gc,
> +                                                 libxl__json_object *param,
> +                                                 const char *name,
> +                                                 libxl__json_object *obj)
> +{
> +    // check that every caller of this free the tree !

We use /* ... */.  I'm not sure what the purpose of this comment is.
Do you mean "this is a fixme" ?  Or is that intended to be a doc
comment saying that callers end up owning some object ?  (The return
value?)  Why is this object not from the gc ?

> +    arg = calloc(1, sizeof (libxl__json_map_node));

These allcations should all use the appropriate libxl helpers,
probably.  (Throughout all your patches.)

I'm not sure exactly what this function _does_, though.  It prepares
some "common" parameters for a qmp call, evidently, but what common
parameters ?

OTOH the code seems to add a single parameter to the map ?

> +static libxl__json_object *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(gc, JSON_STRING);
> +    if (!obj)
> +        goto error_nomem;
> +    obj->u.string = strdup(argument);
> +    if (!obj->u.string)
> +        goto error_nomem;
> +
> +    param = qmp_parameters_common(gc, param, name, obj);
> +    if (!param)
> +        goto error_nomem;
> +
> +    return param;
> +error_nomem:
> +    libxl__json_object_free(gc, param);
> +    libxl__json_object_free(gc, obj);
> +    return NULL;
> +}

This is very similar to the same function for bool.  Perhaps it would
be better to separate out the type-specific object creation ?

Ian.

[PATCH 4/6] libxl_qmp: Use qmp_parameters_* functions for param list of a QMP command.

[Anthony PERARD]

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index edbd3b4..8bd56cf 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
  * If a function fail to allocate memomy for one more parameters, then
@@ -687,6 +686,7 @@ error_nomem:
     return NULL;
 }
 
+#if 0
 static libxl__json_object *qmp_parameters_add_bool(libxl__gc *gc,
                                                    libxl__json_object *param,
                                                    const char *name, bool b)
@@ -835,8 +835,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;
 
@@ -849,56 +848,63 @@ 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",
+    args = qmp_parameters_add_string(gc, NULL, "driver", "xen-pci-passthrough");
+    if (!args)
+        goto error_nomem;
+    args = qmp_parameters_add_string(gc, args, "id",
                           libxl__sprintf(gc, PCI_PT_QDEV_ID,
                                          pcidev->bus, pcidev->dev,
                                          pcidev->func));
-    flexarray_append_pair(parameters, "hostaddr", hostaddr);
+    if (!args)
+        goto error_nomem;
+    args = qmp_parameters_add_string(gc, args, "hostaddr", hostaddr);
+    if (!args)
+        goto error_nomem;
     if (pcidev->vdevfn) {
-        flexarray_append_pair(parameters, "addr",
+        args = qmp_parameters_add_string(gc, args, "addr",
                               libxl__sprintf(gc, "%x.%x",
                                              PCI_SLOT(pcidev->vdevfn),
                                              PCI_FUNC(pcidev->vdevfn)));
+        if (!args)
+            goto error_nomem;
     }
-    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;
+error_nomem:
+    libxl__qmp_close(qmp);
+    return ERROR_NOMEM;
 }
 
 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;
+    args = qmp_parameters_add_string(gc, NULL, "id", id);
+    if (!args) {
+        rc = ERROR_NOMEM;
+        goto out;
+    }
 
-    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);
+out:
     libxl__qmp_close(qmp);
     return rc;
 }
@@ -916,31 +922,23 @@ 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);
+    args = qmp_parameters_add_string(gc, NULL, "filename", (char *)filename);
     if (!args) {
         rc = ERROR_NOMEM;
-        goto out2;
+        goto out;
     }
 
-    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;
@@ -949,23 +947,25 @@ out:
 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);
+    args = qmp_parameters_add_string(gc, NULL, "device", device);
     if (!args)
         return ERROR_NOMEM;
+    args = qmp_parameters_add_string(gc, args, "target", target);
+    if (!args)
+        return ERROR_NOMEM;
+    if (arg) {
+        args = qmp_parameters_add_string(gc, args, "arg", arg);
+        if (!args)
+            return ERROR_NOMEM;
+    }
 
-    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

Re: [PATCH 4/6] libxl_qmp: Use qmp_parameters_* functions for param list of a QMP command.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 4/6] libxl_qmp: Use qmp_parameters_* functions for param list of a QMP command."):
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks:

> -    parameters = flexarray_make(6, 1);
> -    flexarray_append_pair(parameters, "driver", "xen-pci-passthrough");
> -    flexarray_append_pair(parameters, "id",
> +    args = qmp_parameters_add_string(gc, NULL, "driver", "xen-pci-passthrough");
> +    if (!args)
> +        goto error_nomem;

This is a rather clumsy use pattern, threading args through all the
time.  If nothing else it might lead to mistakes because the first
call takes NULL whereas subsequent calls take args:

> +    args = qmp_parameters_add_string(gc, args, "id",
>                            libxl__sprintf(gc, PCI_PT_QDEV_ID,
>                                           pcidev->bus, pcidev->dev,
>                                           pcidev->func));

How about inventing functions that would allow
  libxl__json_object *args = NULL;
  libxl__qmp_param_add(gc, &args, "id", libxl__qmp_sprintf(gc, ....);
with perhaps a helper macro that allows
  QMP_PARAM_SPRINTF(args, "id", PCI_PT_QDEV_ID,
                    pcidev->bus, pcidev->dev, pcidev->func));
?

And again there's a lot of unnecessary error handling.

Ian.

[PATCH 5/6] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

[Anthony PERARD]

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      |   27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2176778..5af4f5c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1353,6 +1353,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 8bd56cf..2c5559f 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -686,7 +686,6 @@ error_nomem:
     return NULL;
 }
 
-#if 0
 static libxl__json_object *qmp_parameters_add_bool(libxl__gc *gc,
                                                    libxl__json_object *param,
                                                    const char *name, bool b)
@@ -707,7 +706,6 @@ error_nomem:
     libxl__json_object_free(gc, obj);
     return NULL;
 }
-#endif
 
 /*
  * API
@@ -1001,6 +999,31 @@ int libxl__qmp_resume(libxl__gc *gc, int domid)
     return rc;
 }
 
+int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable)
+{
+    libxl__qmp_handler *qmp = NULL;
+    libxl__json_object *args = NULL;
+    int rc = 0;
+
+    qmp = libxl__qmp_initialize(gc, domid);
+    if (!qmp)
+        return ERROR_FAIL;
+
+    args = qmp_parameters_add_bool(gc, NULL, "enable", enable);
+    if (!args) {
+        rc = ERROR_NOMEM;
+        goto out;
+    }
+
+    rc = qmp_synchronous_send(qmp, "xen-set-global-dirty-log", args,
+                              NULL, NULL, qmp->timeout);
+    libxl__json_object_free(gc, args);
+
+out:
+    libxl__qmp_close(qmp);
+    return rc;
+}
+
 int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
                                const libxl_domain_config *guest_config)
 {
-- 
Anthony PERARD

Re: [PATCH 5/6] libxl_qmp: Introduce libxl__qmp_set_global_dirty_log.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 5/6] 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.

Again we have the possible concurrent use of the qmp to contend with.

Also:

> +int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable)
> +{
> +    libxl__qmp_handler *qmp = NULL;
> +    libxl__json_object *args = NULL;
> +    int rc = 0;
> +
> +    qmp = libxl__qmp_initialize(gc, domid);
> +    if (!qmp)
> +        return ERROR_FAIL;
> +
> +    args = qmp_parameters_add_bool(gc, NULL, "enable", enable);
> +    if (!args) {
> +        rc = ERROR_NOMEM;
> +        goto out;
> +    }
> +
> +    rc = qmp_synchronous_send(qmp, "xen-set-global-dirty-log", args,
> +                              NULL, NULL, qmp->timeout);
> +    libxl__json_object_free(gc, args);
> +
> +out:
> +    libxl__qmp_close(qmp);
> +    return rc;
> +}

This pattern which is starting to appear multiple times could perhaps
be encapsulated in a function ?

Ian.

[PATCH 6/6] libxl_dom: Call the right switch logdirty for the right DM.

[Anthony PERARD]

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 |   41 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index cecda9b..ee7dc62 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -561,7 +561,7 @@ static void logdirty_init(libxl__logdirty_switch *lds)
     libxl__ev_time_init(&lds->timeout);
 }
 
-void libxl__domain_suspend_common_switch_qemu_logdirty
+static void domain_suspend_switch_qemu_xen_traditional_logdirty
                                (int domid, unsigned enable, void *user)
 {
     libxl__save_helper_state *shs = user;
@@ -631,6 +631,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, 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);
+    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, user);
+        break;
+    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+        domain_suspend_switch_qemu_xen_logdirty(domid, enable, user);
+        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

Re: [PATCH 6/6] libxl_dom: Call the right switch logdirty for the right DM.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 6/6] 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.
...
> -void libxl__domain_suspend_common_switch_qemu_logdirty
> +static void domain_suspend_switch_qemu_xen_traditional_logdirty
>                                 (int domid, unsigned enable, void *user)

If you make these functions internal you should not have them each
separately upcast the void*.  They should take the shs as a typed
pointer.

Apart from that it looks sane.

Thanks,
Ian.