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 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 (2):
libxl: Have flexarray using the GC
libxl_json: Use libxl alloc function.
tools/libxl/flexarray.c | 48 ++++++++++++++---------
tools/libxl/flexarray.h | 7 +++-
tools/libxl/libxl.c | 99 ++++++++++--------------------------------------
tools/libxl/libxl_dm.c | 15 ++------
tools/libxl/libxl_json.c | 93 ++++++++++-----------------------------------
tools/libxl/libxl_pci.c | 18 ++-------
tools/libxl/libxl_qmp.c | 29 ++------------
7 files changed, 83 insertions(+), 226 deletions(-)
--
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 | 48 ++++++++++++++---------
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, 65 insertions(+), 158 deletions(-)
diff --git a/tools/libxl/flexarray.c b/tools/libxl/flexarray.c
index edf616c..ab74c51 100644
--- a/tools/libxl/flexarray.c
+++ b/tools/libxl/flexarray.c
@@ -16,36 +16,48 @@
#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;
}
+static int gc_is_real(const libxl__gc *gc)
+{
+ return gc->alloc_maxsize >= 0;
+}
+
void flexarray_free(flexarray_t *array)
{
+ assert(!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 +67,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 +115,8 @@ 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..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
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>
---
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
Anthony PERARD writes ("[PATCH V2 1/2] libxl: Have flexarray using the
GC"):> This patch makes the flexarray function libxl__gc aware.
> +/*
> + * 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.
> + */
Thanks, I think this explanation is correct.
> +static int gc_is_real(const libxl__gc *gc)
> +{
> + return gc->alloc_maxsize >= 0;
> +}
You have cloned-and-hacked this from libxl_internal.c ! Instead,
either declare it in libxl_internal.h, or move it there and make it
"static inline".
> @@ -104,7 +115,8 @@ void **flexarray_contents(flexarray_t *array)
> {
> void **data;
> data = array->data;
> - free(array);
> + if (!gc_is_real(array->gc))
> + free(array);
> return data;
What an odd function. However, your patch to it seems right to me.
> diff --git a/tools/libxl/flexarray.h b/tools/libxl/flexarray.h
> index ae17f3b..aade417 100644
And the rest of the patch is mostly an impressive number of
deletions. I won''t check the semantics of each one but the style
seems plausible.
Thanks!
Ian.
Ian Jackson
2012-Oct-04 17:32 UTC
Re: [PATCH V2 2/2] libxl_json: Use libxl alloc function.
Anthony PERARD writes ("[PATCH V2 2/2] libxl_json: Use libxl alloc
function."):> This patch makes use of the libxl allocation API and the GC and removes the
> check for allocation failure.
I haven''t checked this line-by-line but it looks plausible.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
Anthony PERARD
2012-Oct-04 19:59 UTC
Re: [PATCH V2 1/2] libxl: Have flexarray using the GC
On Thu, Oct 4, 2012 at 6:31 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> >> +static int gc_is_real(const libxl__gc *gc) >> +{ >> + return gc->alloc_maxsize >= 0; >> +} > > You have cloned-and-hacked this from libxl_internal.c ! Instead, > either declare it in libxl_internal.h, or move it there and make it > "static inline".OK, I''ll "static inline" it.>> @@ -104,7 +115,8 @@ void **flexarray_contents(flexarray_t *array) >> { >> void **data; >> data = array->data; >> - free(array); >> + if (!gc_is_real(array->gc)) >> + free(array); >> return data; > > What an odd function. However, your patch to it seems right to me.It was maybe a workaround the missing ability to be GC''ed of flexarray. Thanks, -- Anthony PERARD