[PATCH V3 0/3] Cleanup: flexarray taking gc.

[PATCH V3 0/3] 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 modify flexarray_make to take gc as argument and update every
user in libxl to pass gc and to not call flexarray_free anymore.

The second one does some cleanup only in libxl_json to make it use the gc.

Change since v2:
  - New patch to expose gc_is_real to libxl.

Change since v1:
  - Change order of the two patch to not end up with leaks.
  - Add a comment on the first patch to tell why it OK to store the gc in the
    flexarray struct.
  - Keep the _free functions (libxl__json_object_free and flexarray_free, as
    they can be used if gc in NOGC.


Anthony PERARD (3):
  libxl: Move gc_is_real to libxl_internal.h.
  libxl: Have flexarray using the GC
  libxl_json: Use libxl alloc function.

 tools/libxl/flexarray.c      | 43 +++++++++++--------
 tools/libxl/flexarray.h      |  7 +++-
 tools/libxl/libxl.c          | 99 +++++++++-----------------------------------
 tools/libxl/libxl_dm.c       | 15 ++-----
 tools/libxl/libxl_internal.c | 11 ++---
 tools/libxl/libxl_internal.h |  5 +++
 tools/libxl/libxl_json.c     | 93 +++++++++--------------------------------
 tools/libxl/libxl_pci.c      | 18 ++------
 tools/libxl/libxl_qmp.c      | 29 ++-----------
 9 files changed, 86 insertions(+), 234 deletions(-)

-- 
Anthony PERARD

[PATCH V3 1/3] libxl: Move gc_is_real to libxl_internal.h.

[Anthony PERARD]

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

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 211c8f5..5a8cd38 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -30,16 +30,11 @@ void libxl__alloc_failed(libxl_ctx *ctx, const char *func,
 #undef L
 }
 
-static int gc_is_real(const libxl__gc *gc)
-{
-    return gc->alloc_maxsize >= 0;
-}
-
 void libxl__ptr_add(libxl__gc *gc, void *ptr)
 {
     int i;
 
-    if (!gc_is_real(gc))
+    if (!libxl__gc_is_real(gc))
         return;
 
     if (!ptr)
@@ -71,7 +66,7 @@ void libxl__free_all(libxl__gc *gc)
     void *ptr;
     int i;
 
-    assert(gc_is_real(gc));
+    assert(libxl__gc_is_real(gc));
 
     for (i = 0; i < gc->alloc_maxsize; i++) {
         ptr = gc->alloc_ptrs[i];
@@ -111,7 +106,7 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
 
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
-    } else if (new_ptr != ptr && gc_is_real(gc)) {
+    } else if (new_ptr != ptr && libxl__gc_is_real(gc)) {
         for (i = 0; i < gc->alloc_maxsize; i++) {
             if (gc->alloc_ptrs[i] == ptr) {
                 gc->alloc_ptrs[i] = new_ptr;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b6f54ba..c0e879d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -446,6 +446,11 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
     return gc->owner;
 }
 
+static inline int libxl__gc_is_real(const libxl__gc *gc)
+{
+    return gc->alloc_maxsize >= 0;
+}
+
 /*
  * Memory allocation tracking/helpers
  *
-- 
Anthony PERARD

Re: [PATCH V3 1/3] libxl: Move gc_is_real to libxl_internal.h.

[Ian Jackson]

Anthony PERARD writes ("[PATCH V3 1/3] libxl: Move gc_is_real to libxl_internal.h."):
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(Strictly speaking there is no need to rename the function if it's
going to be static inline, but it's fine to do so.)

Ian.

[PATCH V3 2/3] 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  | 43 ++++++++++++---------
 tools/libxl/flexarray.h  |  7 +++-
 tools/libxl/libxl.c      | 99 ++++++++++--------------------------------------
 tools/libxl/libxl_dm.c   | 15 ++------
 tools/libxl/libxl_json.c |  8 +---
 tools/libxl/libxl_pci.c  | 18 ++-------
 tools/libxl/libxl_qmp.c  | 28 ++------------
 7 files changed, 60 insertions(+), 158 deletions(-)

diff --git a/tools/libxl/flexarray.c b/tools/libxl/flexarray.c
index edf616c..fe40e76 100644
--- a/tools/libxl/flexarray.c
+++ b/tools/libxl/flexarray.c
@@ -16,36 +16,43 @@
 #include "libxl_internal.h"
 #include <stdarg.h>
 
-flexarray_t *flexarray_make(int size, int autogrow)
+/*
+ * It is safe to store gc in the struct because:
+ * - If it an actual gc, then the flexarray should not be used after the gc
+ *   have been freed.
+ * - If it is a NOGC, then this point to a structure embedded in libxl_ctx,
+ *   therefore will survive across several libxl calls.
+ */
+
+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 *));
-    }
+    flexarray_t *array;
+
+    GCNEW(array);
+    array->size = size;
+    array->autogrow = autogrow;
+    array->count = 0;
+    array->gc = gc;
+    GCNEW_ARRAY(array->data, size);
+
     return array;
 }
 
 void flexarray_free(flexarray_t *array)
 {
+    assert(!libxl__gc_is_real(array->gc));
     free(array->data);
     free(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 +62,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;
@@ -104,7 +110,8 @@ void **flexarray_contents(flexarray_t *array)
 {
     void **data;
     data = array->data;
-    free(array);
+    if (!libxl__gc_is_real(array->gc))
+        free(array);
     return data;
 }
 
diff --git a/tools/libxl/flexarray.h b/tools/libxl/flexarray.h
index ae17f3b..aade417 100644
--- a/tools/libxl/flexarray.h
+++ b/tools/libxl/flexarray.h
@@ -16,16 +16,19 @@
 #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 flexarray_t *flexarray_make(struct libxl__gc *gc, int size, int autogrow);
 _hidden void flexarray_free(flexarray_t *array);
-_hidden int flexarray_grow(flexarray_t *array, int extents);
+_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 8e17842..2e6322b 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -220,13 +220,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);
-        if (array == NULL) {
-            LIBXL__LOG_ERRNO(libxl__gc_owner(gc), LIBXL__LOG_ERROR,
-                             "Failed to allocate a flexarray");
-            free(obj);
-            return NULL;
-        }
+        flexarray_t *array = flexarray_make(NOGC, 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 2c4d269..6bdf924 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -763,7 +763,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,
@@ -777,8 +777,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);
@@ -787,7 +785,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;
 }
@@ -803,16 +800,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;
 }
@@ -838,24 +832,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;
 }
@@ -867,19 +850,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 V3 2/3] libxl: Have flexarray using the GC

[Ian Jackson]

Anthony PERARD writes ("[PATCH V3 2/3] libxl: Have flexarray using the GC"):
> This patch makes the flexarray function libxl__gc aware.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH V3 3/3] libxl_json: Use libxl alloc function.

[Anthony PERARD]

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

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_json.c | 87 +++++++++++-------------------------------------
 tools/libxl/libxl_qmp.c  |  1 -
 2 files changed, 19 insertions(+), 69 deletions(-)

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 2e6322b..3cba48f 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -210,17 +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(NOGC, 1, 1);
+        flexarray_t *array = flexarray_make(gc, 1, 1);
         if (type == JSON_MAP)
             obj->u.map = array;
         else
@@ -250,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,
@@ -387,11 +378,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;
     }
 
@@ -405,12 +394,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;
     }
 
@@ -442,8 +429,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);
@@ -452,23 +438,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;
 
@@ -476,7 +455,6 @@ error:
 
 out:
     if (json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
-        libxl__json_object_free(ctx->gc, obj);
         return 0;
     }
 
@@ -490,26 +468,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;
     }
 
@@ -522,13 +491,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);
 
@@ -536,21 +501,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");
@@ -567,12 +524,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;
         }
     }
@@ -609,12 +564,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;
         }
     }
@@ -704,8 +657,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 6bdf924..15550e7 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 V3 0/3] Cleanup: flexarray taking gc.

[Ian Campbell]

On Fri, 2012-10-05 at 10:04 +0100, Anthony PERARD wrote:
> This two patches do a bit of cleanup in the memomy managment in libxl,
> regarding the use of flexarray.

Applied all 3 with IanJ's acks.

Thanks.