[PATCH 0/2] Cleanup: flexarray taking gc.

[PATCH 0/2] Cleanup: flexarray taking gc.

[Anthony PERARD]

This two patches do a bit of cleanup in the memomy managment in libxl,
regarding the use of flexarray.

The first one does some cleanup only in libxl_json.

The second one modify flexarray_make to take gc as argument and update every
user in libxl to pass gc and to not call flexarray_free anymore.


Anthony PERARD (2):
  libxl_json: Use libxl alloc function
  libxl: Have flexarray using the GC

 tools/libxl/flexarray.c      |  45 +++++++-------
 tools/libxl/flexarray.h      |   8 ++-
 tools/libxl/libxl.c          |  99 ++++++-------------------------
 tools/libxl/libxl_dm.c       |  15 +----
 tools/libxl/libxl_internal.h |   1 -
 tools/libxl/libxl_json.c     | 137 ++++++-------------------------------------
 tools/libxl/libxl_pci.c      |  18 +-----
 tools/libxl/libxl_qmp.c      |  29 ++-------
 8 files changed, 74 insertions(+), 278 deletions(-)

-- 
Anthony PERARD

[PATCH 1/2] libxl_json: Use libxl alloc function

[Anthony PERARD]

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

This patch also assume that flexarray does not need to be freed as it will be
gc'd in the next patch.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b6f54ba..55fa771 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1519,7 +1519,6 @@ 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 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 8e17842..25df4a9 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -210,23 +210,12 @@ 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(gc, sizeof(*obj));
 
     obj->type = type;
 
     if (type == JSON_MAP || type == JSON_ARRAY) {
         flexarray_t *array = flexarray_make(1, 1);
-        if (array == NULL) {
-            LIBXL__LOG_ERRNO(libxl__gc_owner(gc), LIBXL__LOG_ERROR,
-                             "Failed to allocate a flexarray");
-            free(obj);
-            return NULL;
-        }
         if (type == JSON_MAP)
             obj->u.map = array;
         else
@@ -256,11 +245,7 @@ static int json_object_append_to(libxl__gc *gc,
         break;
     }
     case JSON_ARRAY:
-        if (flexarray_append(dst->u.array, obj) == 2) {
-            LIBXL__LOG_ERRNO(libxl__gc_owner(gc), LIBXL__LOG_ERROR,
-                             "Failed to grow a flexarray");
-            return -1;
-        }
+        flexarray_append(dst->u.array, obj);
         break;
     default:
         LIBXL__LOG(libxl__gc_owner(gc), LIBXL__LOG_ERROR,
@@ -273,50 +258,6 @@ static int json_object_append_to(libxl__gc *gc,
     return 0;
 }
 
-void libxl__json_object_free(libxl__gc *gc, libxl__json_object *obj)
-{
-    int idx = 0;
-
-    if (obj == NULL)
-        return;
-    switch (obj->type) {
-    case JSON_STRING:
-    case JSON_NUMBER:
-        free(obj->u.string);
-        break;
-    case JSON_MAP: {
-        libxl__json_map_node *node = NULL;
-
-        for (idx = 0; idx < obj->u.map->count; idx++) {
-            if (flexarray_get(obj->u.map, idx, (void**)&node) != 0)
-                break;
-            libxl__json_object_free(gc, node->obj);
-            free(node->map_key);
-            free(node);
-            node = NULL;
-        }
-        flexarray_free(obj->u.map);
-        break;
-    }
-    case JSON_ARRAY: {
-        libxl__json_object *node = NULL;
-        break;
-
-        for (idx = 0; idx < obj->u.array->count; idx++) {
-            if (flexarray_get(obj->u.array, idx, (void**)&node) != 0)
-                break;
-            libxl__json_object_free(gc, node);
-            node = NULL;
-        }
-        flexarray_free(obj->u.array);
-        break;
-    }
-    default:
-        break;
-    }
-    free(obj);
-}
-
 libxl__json_object *libxl__json_array_get(const libxl__json_object *o, int i)
 {
     flexarray_t *array = NULL;
@@ -393,11 +334,9 @@ 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);
         return 0;
     }
 
@@ -411,12 +350,10 @@ 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);
         return 0;
     }
 
@@ -448,8 +385,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,23 +394,16 @@ 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) {
-        LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc), LIBXL__LOG_ERROR,
-                         "Failed to allocate");
-        return 0;
-    }
+    t = libxl__zalloc(ctx->gc, len + 1);
     strncpy(t, s, len);
     t[len] = 0;
 
@@ -482,7 +411,6 @@ error:
 
 out:
     if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
-        libxl__json_object_free(ctx->gc, obj);
         return 0;
     }
 
@@ -496,26 +424,17 @@ static int json_callback_string(void *opaque, const unsigned char *str,
     char *t = NULL;
     libxl__json_object *obj = NULL;
 
-    t = malloc(len + 1);
-    if (t == NULL) {
-        LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc), LIBXL__LOG_ERROR,
-                         "Failed to allocate");
-        return 0;
-    }
+    t = libxl__zalloc(ctx->gc, len + 1);
 
     DEBUG_GEN_STRING(ctx, str, len);
 
     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) {
-        libxl__json_object_free(ctx->gc, obj);
         return 0;
     }
 
@@ -528,13 +447,9 @@ static int json_callback_map_key(void *opaque, const unsigned char *str,
     libxl__yajl_ctx *ctx = opaque;
     char *t = NULL;
     libxl__json_object *obj = ctx->current;
+    libxl__gc *gc = ctx->gc;
 
-    t = malloc(len + 1);
-    if (t == NULL) {
-        LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc), LIBXL__LOG_ERROR,
-                         "Failed to allocate");
-        return 0;
-    }
+    t = libxl__zalloc(gc, len + 1);
 
     DEBUG_GEN_STRING(ctx, str, len);
 
@@ -542,21 +457,13 @@ static int json_callback_map_key(void *opaque, const unsigned char *str,
     t[len] = 0;
 
     if (libxl__json_object_is_map(obj)) {
-        libxl__json_map_node *node = malloc(sizeof (libxl__json_map_node));
-        if (node == NULL) {
-            LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc), LIBXL__LOG_ERROR,
-                             "Failed to allocate");
-            return 0;
-        }
+        libxl__json_map_node *node;
 
+        GCNEW(node);
         node->map_key = t;
         node->obj = NULL;
 
-        if (flexarray_append(obj->u.map, node) == 2) {
-            LIBXL__LOG_ERRNO(libxl__gc_owner(ctx->gc), LIBXL__LOG_ERROR,
-                             "Failed to grow a flexarray");
-            return 0;
-        }
+        flexarray_append(obj->u.map, node);
     } else {
         LIBXL__LOG(libxl__gc_owner(ctx->gc), LIBXL__LOG_ERROR,
                    "Current json object is not a map");
@@ -573,12 +480,10 @@ 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) {
-            libxl__json_object_free(ctx->gc, obj);
             return 0;
         }
     }
@@ -615,12 +520,10 @@ 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) {
-            libxl__json_object_free(ctx->gc, obj);
             return 0;
         }
     }
@@ -710,8 +613,6 @@ out:
 
     LIBXL__LOG(libxl__gc_owner(gc), LIBXL__LOG_ERROR, "yajl error: %s", str);
     yajl_free_error(yajl_ctx.hand, str);
-
-    libxl__json_object_free(gc, yajl_ctx.head);
     yajl_ctx_free(&yajl_ctx);
     return NULL;
 }
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 2c4d269..0757fc2 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -484,7 +484,6 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
 
                 if (o) {
                     rc = qmp_handle_response(qmp, o);
-                    libxl__json_object_free(gc, o);
                 } else {
                     LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
                                "Parse error of : %s\n", s);
-- 
Anthony PERARD

Re: [PATCH 1/2] libxl_json: Use libxl alloc function

[Ian Jackson]

Anthony PERARD writes ("[Xen-devel] [PATCH 1/2] libxl_json: Use libxl alloc function"):
> This patch makes use of the libxl allocation API and removes the check for
> allocation failure.
> 
> This patch also assume that flexarray does not need to be freed as it will be
> gc'd in the next patch.

Is this really the right way to structure this series ?  It seems that
after applying only the first, the code has a memory leak.

Ian.

Re: [PATCH 1/2] libxl_json: Use libxl alloc function

[Anthony PERARD]

On 09/28/2012 06:22 PM, Ian Jackson wrote:
> Anthony PERARD writes ("[Xen-devel] [PATCH 1/2] libxl_json: Use libxl alloc function"):
>> This patch makes use of the libxl allocation API and removes the check for
>> allocation failure.
>>
>> This patch also assume that flexarray does not need to be freed as it will be
>> gc'd in the next patch.
> 
> Is this really the right way to structure this series ?  It seems that
> after applying only the first, the code has a memory leak.

Yes, there is a memory leak if only this patch is applied. I'll find a
better way to structure this patch series.

-- 
Anthony PERARD

[PATCH 2/2] libxl: Have flexarray using the GC

[Anthony PERARD]

This patch makes the flexarray function libxl__gc aware.

It also updates every function that use a flexarray to pass the gc and removes
every memory allocation check and free.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/flexarray.c  | 45 ++++++++++------------
 tools/libxl/flexarray.h  |  8 ++--
 tools/libxl/libxl.c      | 99 ++++++++++--------------------------------------
 tools/libxl/libxl_dm.c   | 15 ++------
 tools/libxl/libxl_json.c |  2 +-
 tools/libxl/libxl_pci.c  | 18 ++-------
 tools/libxl/libxl_qmp.c  | 28 ++------------
 7 files changed, 56 insertions(+), 159 deletions(-)

diff --git a/tools/libxl/flexarray.c b/tools/libxl/flexarray.c
index edf616c..1c2de1b 100644
--- a/tools/libxl/flexarray.c
+++ b/tools/libxl/flexarray.c
@@ -16,36 +16,28 @@
 #include "libxl_internal.h"
 #include <stdarg.h>
 
-flexarray_t *flexarray_make(int size, int autogrow)
+flexarray_t *flexarray_make(libxl__gc *gc, int size, int autogrow)
 {
-    flexarray_t *array = malloc(sizeof(struct flexarray));
-    if (array) {
-        array->size = size;
-        array->autogrow = autogrow;
-        array->count = 0;
-        array->data = calloc(size, sizeof(void *));
-    }
-    return array;
-}
+    flexarray_t *array;
 
-void flexarray_free(flexarray_t *array)
-{
-    free(array->data);
-    free(array);
+    GCNEW(array);
+    array->size = size;
+    array->autogrow = autogrow;
+    array->count = 0;
+    array->gc = gc;
+    GCNEW_ARRAY(array->data, size);
+
+    return array;
 }
 
-int flexarray_grow(flexarray_t *array, int extents)
+void flexarray_grow(flexarray_t *array, int extents)
 {
-    void **data;
     int newsize;
+    libxl__gc *gc = array->gc;
 
     newsize = array->size + extents;
-    data = realloc(array->data, sizeof(void *) * newsize);
-    if (!data)
-        return 1;
+    GCREALLOC_ARRAY(array->data, newsize);
     array->size += extents;
-    array->data = data;
-    return 0;
 }
 
 int flexarray_set(flexarray_t *array, unsigned int idx, void *ptr)
@@ -55,8 +47,7 @@ int flexarray_set(flexarray_t *array, unsigned int idx, void *ptr)
         if (!array->autogrow)
             return 1;
         newsize = (array->size * 2 < idx) ? idx + 1 : array->size * 2;
-        if (flexarray_grow(array, newsize - array->size))
-            return 2;
+        flexarray_grow(array, newsize - array->size);
     }
     if ( idx + 1 > array->count )
         array->count = idx + 1;
@@ -100,11 +91,17 @@ int flexarray_get(flexarray_t *array, int idx, void **ptr)
     return 0;
 }
 
+static int gc_is_real(const libxl__gc *gc)
+{
+    return gc->alloc_maxsize >= 0;
+}
+
 void **flexarray_contents(flexarray_t *array)
 {
     void **data;
     data = array->data;
-    free(array);
+    if (!gc_is_real(array->gc))
+        free(array);
     return data;
 }
 
diff --git a/tools/libxl/flexarray.h b/tools/libxl/flexarray.h
index ae17f3b..7e2bc5a 100644
--- a/tools/libxl/flexarray.h
+++ b/tools/libxl/flexarray.h
@@ -16,16 +16,18 @@
 #ifndef FLEXARRAY_H
 #define FLEXARRAY_H
 
+struct libxl__gc;
+
 typedef struct flexarray {
     int size;
     int autogrow;
     unsigned int count;
     void **data; /* array of pointer */
+    struct libxl__gc *gc;
 } flexarray_t;
 
-_hidden flexarray_t *flexarray_make(int size, int autogrow);
-_hidden void flexarray_free(flexarray_t *array);
-_hidden int flexarray_grow(flexarray_t *array, int extents);
+_hidden flexarray_t *flexarray_make(struct libxl__gc *gc, int size, int autogrow);
+_hidden void flexarray_grow(flexarray_t *array, int extents);
 _hidden int flexarray_set(flexarray_t *array, unsigned int index, void *ptr);
 _hidden int flexarray_append(flexarray_t *array, void *ptr);
 _hidden int flexarray_append_pair(flexarray_t *array, void *ptr1, void *ptr2);
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1606eb1..af3682f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1820,27 +1820,15 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         rc = libxl__device_disk_setdefault(gc, disk);
         if (rc) goto out;
 
-        if (front)
-            flexarray_free(front);
-        front = flexarray_make(16, 1);
-        if (!front) {
-            rc = ERROR_NOMEM;
-            goto out;
-        }
-        if (back)
-            flexarray_free(back);
-        back = flexarray_make(16, 1);
-        if (!back) {
-            rc = ERROR_NOMEM;
-            goto out_free;
-        }
+        front = flexarray_make(gc, 16, 1);
+        back = flexarray_make(gc, 16, 1);
 
         GCNEW(device);
         rc = libxl__device_from_disk(gc, domid, disk, device);
         if (rc != 0) {
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
                    " virtual disk identifier %s", disk->vdev);
-            goto out_free;
+            goto out;
         }
 
         switch (disk->backend) {
@@ -1878,7 +1866,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                     LOG(ERROR, "failed to get blktap devpath for %p\n",
                         disk->pdev_path);
                     rc = ERROR_FAIL;
-                    goto out_free;
+                    goto out;
                 }
                 flexarray_append(back, "tapdisk-params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",
@@ -1900,7 +1888,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
             default:
                 LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
                 rc = ERROR_INVAL;
-                goto out_free;
+                goto out;
         }
 
         flexarray_append(back, "frontend-id");
@@ -1937,7 +1925,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
-        if (rc < 0) goto out_free;
+        if (rc < 0) goto out;
     }
 
     aodev->dev = device;
@@ -1946,9 +1934,6 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
 
     rc = 0;
 
-out_free:
-    flexarray_free(back);
-    flexarray_free(front);
 out:
     libxl__xs_transaction_abort(gc, &t);
     aodev->rc = rc;
@@ -2204,7 +2189,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     if (rc) goto out;
     path = libxl__device_backend_path(gc, &device);
 
-    insert = flexarray_make(4, 1);
+    insert = flexarray_make(gc, 4, 1);
 
     flexarray_append_pair(insert, "type",
                           libxl__device_disk_string_of_backend(disk->backend));
@@ -2230,8 +2215,6 @@ out:
         libxl_device_disk_dispose(&disks[i]);
     free(disks);
 
-    if (insert) flexarray_free(insert);
-
     if (rc) return AO_ABORT(rc);
     return AO_INPROGRESS;
 }
@@ -2567,21 +2550,13 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
     rc = libxl__device_nic_setdefault(gc, nic, domid);
     if (rc) goto out;
 
-    front = flexarray_make(16, 1);
-    if (!front) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
-    back = flexarray_make(16, 1);
-    if (!back) {
-        rc = ERROR_NOMEM;
-        goto out_free;
-    }
+    front = flexarray_make(gc, 16, 1);
+    back = flexarray_make(gc, 16, 1);
 
     if (nic->devid == -1) {
         if (!(dompath = libxl__xs_get_dompath(gc, domid))) {
             rc = ERROR_FAIL;
-            goto out_free;
+            goto out;
         }
         if (!(l = libxl__xs_directory(gc, XBT_NULL,
                                      libxl__sprintf(gc, "%s/device/vif", dompath), &nb))) {
@@ -2593,7 +2568,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
 
     GCNEW(device);
     rc = libxl__device_from_nic(gc, domid, nic, device);
-    if ( rc != 0 ) goto out_free;
+    if ( rc != 0 ) goto out;
 
     flexarray_append(back, "frontend-id");
     flexarray_append(back, libxl__sprintf(gc, "%d", domid));
@@ -2652,9 +2627,6 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
     libxl__wait_device_connection(egc, aodev);
 
     rc = 0;
-out_free:
-    flexarray_free(back);
-    flexarray_free(front);
 out:
     aodev->rc = rc;
     if (rc) aodev->callback(egc, aodev);
@@ -2851,16 +2823,8 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         goto out;
     }
 
-    front = flexarray_make(16, 1);
-    if (!front) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
-    back = flexarray_make(16, 1);
-    if (!back) {
-        rc = ERROR_NOMEM;
-        goto out_free;
-    }
+    front = flexarray_make(gc, 16, 1);
+    back = flexarray_make(gc, 16, 1);
 
     device.backend_devid = console->devid;
     device.backend_domid = console->backend_domid;
@@ -2908,9 +2872,6 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
     rc = 0;
-out_free:
-    flexarray_free(back);
-    flexarray_free(front);
 out:
     return rc;
 }
@@ -2964,19 +2925,11 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
     rc = libxl__device_vkb_setdefault(gc, vkb);
     if (rc) goto out;
 
-    front = flexarray_make(16, 1);
-    if (!front) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
-    back = flexarray_make(16, 1);
-    if (!back) {
-        rc = ERROR_NOMEM;
-        goto out_free;
-    }
+    front = flexarray_make(gc, 16, 1);
+    back = flexarray_make(gc, 16, 1);
 
     rc = libxl__device_from_vkb(gc, domid, vkb, &device);
-    if (rc != 0) goto out_free;
+    if (rc != 0) goto out;
 
     flexarray_append(back, "frontend-id");
     flexarray_append(back, libxl__sprintf(gc, "%d", domid));
@@ -2996,9 +2949,6 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
     rc = 0;
-out_free:
-    flexarray_free(back);
-    flexarray_free(front);
 out:
     return rc;
 }
@@ -3063,19 +3013,11 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb)
     rc = libxl__device_vfb_setdefault(gc, vfb);
     if (rc) goto out;
 
-    front = flexarray_make(16, 1);
-    if (!front) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
-    back = flexarray_make(16, 1);
-    if (!back) {
-        rc = ERROR_NOMEM;
-        goto out_free;
-    }
+    front = flexarray_make(gc, 16, 1);
+    back = flexarray_make(gc, 16, 1);
 
     rc = libxl__device_from_vfb(gc, domid, vfb, &device);
-    if (rc != 0) goto out_free;
+    if (rc != 0) goto out;
 
     flexarray_append_pair(back, "frontend-id", libxl__sprintf(gc, "%d", domid));
     flexarray_append_pair(back, "online", "1");
@@ -3108,9 +3050,6 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb)
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
     rc = 0;
-out_free:
-    flexarray_free(front);
-    flexarray_free(back);
 out:
     return rc;
 }
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3be7bad..82d2009 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -109,10 +109,7 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
     const char *keymap = dm_keymap(guest_config);
     int i;
     flexarray_t *dm_args;
-    dm_args = flexarray_make(16, 1);
-
-    if (!dm_args)
-        return NULL;
+    dm_args = flexarray_make(gc, 16, 1);
 
     flexarray_vappend(dm_args, dm,
                       "-d", libxl__sprintf(gc, "%d", domid), NULL);
@@ -340,9 +337,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     int i;
     uint64_t ram_size;
 
-    dm_args = flexarray_make(16, 1);
-    if (!dm_args)
-        return NULL;
+    dm_args = flexarray_make(gc, 16, 1);
 
     flexarray_vappend(dm_args, dm,
                       "-xen-domid",
@@ -837,7 +832,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
                          "setting target domain %d -> %d",
                          dm_domid, guest_domid);
         ret = ERROR_FAIL;
-        goto out_free;
+        goto out;
     }
     xs_set_target(ctx->xsh, dm_domid, guest_domid);
 
@@ -861,11 +856,8 @@ retry_transaction:
     libxl__add_disks(egc, ao, dm_domid, dm_config, &sdss->multidev);
     libxl__multidev_prepared(egc, &sdss->multidev, 0);
 
-    free(args);
     return;
 
-out_free:
-    free(args);
 out:
     assert(ret);
     spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
@@ -1165,7 +1157,6 @@ retry_transaction:
 out_close:
     close(null);
     close(logfile_w);
-    free(args);
 out:
     if (rc)
         device_model_spawn_outcome(egc, dmss, rc);
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 25df4a9..52fed51 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -215,7 +215,7 @@ static libxl__json_object *json_object_alloc(libxl__gc *gc,
     obj->type = type;
 
     if (type == JSON_MAP || type == JSON_ARRAY) {
-        flexarray_t *array = flexarray_make(1, 1);
+        flexarray_t *array = flexarray_make(gc, 1, 1);
         if (type == JSON_MAP)
             obj->u.map = array;
         else
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index ff447e7..eac35c1 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -73,12 +73,8 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     libxl__device device;
     int ret = ERROR_NOMEM, i;
 
-    front = flexarray_make(16, 1);
-    if (!front)
-        goto out;
-    back = flexarray_make(16, 1);
-    if (!back)
-        goto out;
+    front = flexarray_make(gc, 16, 1);
+    back = flexarray_make(gc, 16, 1);
 
     ret = 0;
 
@@ -108,11 +104,6 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count));
 
-out:
-    if (back)
-        flexarray_free(back);
-    if (front)
-        flexarray_free(front);
     return 0;
 }
 
@@ -138,9 +129,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
             return ERROR_FAIL;
     }
 
-    back = flexarray_make(16, 1);
-    if (!back)
-        return ERROR_NOMEM;
+    back = flexarray_make(gc, 16, 1);
 
     LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new pci device to xenstore");
     num = atoi(num_devs);
@@ -157,7 +146,6 @@ retry_transaction:
         if (errno == EAGAIN)
             goto retry_transaction;
 
-    flexarray_free(back);
     return 0;
 }
 
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 0757fc2..15550e7 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -762,7 +762,7 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
     if (!hostaddr)
         return -1;
 
-    parameters = flexarray_make(6, 1);
+    parameters = flexarray_make(gc, 6, 1);
     flexarray_append_pair(parameters, "driver", "xen-pci-passthrough");
     flexarray_append_pair(parameters, "id",
                           libxl__sprintf(gc, PCI_PT_QDEV_ID,
@@ -776,8 +776,6 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
                                              PCI_FUNC(pcidev->vdevfn)));
     }
     args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
-    if (!args)
-        return -1;
 
     rc = qmp_synchronous_send(qmp, "device_add", &args,
                               NULL, NULL, qmp->timeout);
@@ -786,7 +784,6 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
                                   pci_add_callback, pcidev, qmp->timeout);
     }
 
-    flexarray_free(parameters);
     libxl__qmp_close(qmp);
     return rc;
 }
@@ -802,16 +799,13 @@ static int qmp_device_del(libxl__gc *gc, int domid, char *id)
     if (!qmp)
         return ERROR_FAIL;
 
-    parameters = flexarray_make(2, 1);
+    parameters = flexarray_make(gc, 2, 1);
     flexarray_append_pair(parameters, "id", id);
     args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
-    if (!args)
-        return ERROR_NOMEM;
 
     rc = qmp_synchronous_send(qmp, "device_del", &args,
                               NULL, NULL, qmp->timeout);
 
-    flexarray_free(parameters);
     libxl__qmp_close(qmp);
     return rc;
 }
@@ -837,24 +831,13 @@ int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename)
     if (!qmp)
         return ERROR_FAIL;
 
-    parameters = flexarray_make(2, 1);
-    if (!parameters) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
+    parameters = flexarray_make(gc, 2, 1);
     flexarray_append_pair(parameters, "filename", (char *)filename);
     args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
-    if (!args) {
-        rc = ERROR_NOMEM;
-        goto out2;
-    }
 
     rc = qmp_synchronous_send(qmp, "xen-save-devices-state", &args,
                               NULL, NULL, qmp->timeout);
 
-out2:
-    flexarray_free(parameters);
-out:
     libxl__qmp_close(qmp);
     return rc;
 }
@@ -866,19 +849,16 @@ static int qmp_change(libxl__gc *gc, libxl__qmp_handler *qmp,
     libxl_key_value_list args = NULL;
     int rc = 0;
 
-    parameters = flexarray_make(6, 1);
+    parameters = flexarray_make(gc, 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;
 
     rc = qmp_synchronous_send(qmp, "change", &args,
                               NULL, NULL, qmp->timeout);
 
-    flexarray_free(parameters);
     return rc;
 }
 
-- 
Anthony PERARD

Re: [PATCH 2/2] libxl: Have flexarray using the GC

[Ian Jackson]

Anthony PERARD writes ("[Xen-devel] [PATCH 2/2] libxl: Have flexarray using the GC"):
> This patch makes the flexarray function libxl__gc aware.
...
> -flexarray_t *flexarray_make(int size, int autogrow)
> +flexarray_t *flexarray_make(libxl__gc *gc, int size, int autogrow)
...
>  {
...
> +    array->gc = gc;

If gc is NOGC then this does the wrong thing.  Callers should be able
to specify NOGC for a flexarray which they want to survive across
multiple calls into libxl.

For this all to work correctly, including error handling, I think
flexarray_grow and its callers need to take a gc from the context.  It
would be wise to assert that the either 1. both the gc passed to make
and grow are NOGC or 2. they are the same.

Ian.

Re: [PATCH 2/2] libxl: Have flexarray using the GC

[Anthony PERARD]

On 09/28/2012 06:27 PM, Ian Jackson wrote:
> Anthony PERARD writes ("[Xen-devel] [PATCH 2/2] libxl: Have flexarray
using the GC"):
>> This patch makes the flexarray function libxl__gc aware.
> ...
>> -flexarray_t *flexarray_make(int size, int autogrow)
>> +flexarray_t *flexarray_make(libxl__gc *gc, int size, int autogrow)
> ...
>>  {
> ...
>> +    array->gc = gc;
>
> If gc is NOGC then this does the wrong thing.  Callers should be able
> to specify NOGC for a flexarray which they want to survive across
> multiple calls into libxl.

I'm not sure I see your point here.  If a something needs a NOGC'ed
flexarray, then flexarray_make(NOGC, x, x) will give this.

> For this all to work correctly, including error handling, I think
> flexarray_grow and its callers need to take a gc from the context.  It
> would be wise to assert that the either 1. both the gc passed to make
> and grow are NOGC or 2. they are the same.

All right, I'll add gc to _grow and it's caller. And will assert on both
condition.

Thanks,

-- 
Anthony PERARD

Re: [PATCH 2/2] libxl: Have flexarray using the GC

[Ian Jackson]

Anthony PERARD writes ("Re: [Xen-devel] [PATCH 2/2] libxl: Have flexarray using the GC"):
> On 09/28/2012 06:27 PM, Ian Jackson wrote:
> > If gc is NOGC then this does the wrong thing.  Callers should be able
> > to specify NOGC for a flexarray which they want to survive across
> > multiple calls into libxl.
> 
> I'm not sure I see your point here.  If a something needs a NOGC'ed
> flexarray, then flexarray_make(NOGC, x, x) will give this.

So then you put the flexarray in some long-term data structure, which
survives the call to libxl and therefore the gc.  Is it safe for
NOGC's gc*, derived from the now-destroyed actual gc, to be embedded
in the flexarray and reused later in a different libxl call with a
different gc (but the same ctx) ?

> > For this all to work correctly, including error handling, I think
> > flexarray_grow and its callers need to take a gc from the context.  It
> > would be wise to assert that the either 1. both the gc passed to make
> > and grow are NOGC or 2. they are the same.
> 
> All right, I'll add gc to _grow and it's caller. And will assert on both
> condition.

Feel free instead to explain to me why I'm wrong :-).

Ian.

Re: [PATCH 2/2] libxl: Have flexarray using the GC

[Anthony PERARD]

On 10/01/2012 03:17 PM, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [Xen-devel] [PATCH 2/2] libxl: Have flexarray using the GC"):
>> > On 09/28/2012 06:27 PM, Ian Jackson wrote:
>>> > > If gc is NOGC then this does the wrong thing.  Callers should be able
>>> > > to specify NOGC for a flexarray which they want to survive across
>>> > > multiple calls into libxl.
>> > 
>> > I'm not sure I see your point here.  If a something needs a NOGC'ed
>> > flexarray, then flexarray_make(NOGC, x, x) will give this.
> So then you put the flexarray in some long-term data structure, which
> survives the call to libxl and therefore the gc.  Is it safe for
> NOGC's gc*, derived from the now-destroyed actual gc, to be embedded
> in the flexarray and reused later in a different libxl call with a
> different gc (but the same ctx) ?

Ah, I now see the issue, a pointer that lead nowhere :S.

Thanks,

-- 
Anthony PERARD