Ian.Campbell at citrix
Jun 13, 2012, 6:11 AM
Post #2 of 2
(52 views)
Permalink
|
|
Re: [PATCH 13/19] libxl: Do not pass NULL as gc_opt; introduce NOGC
[In reply to]
|
|
On Fri, 2012-06-08 at 18:34 +0100, Ian Jackson wrote:
> In 25182:6c3345d7e9d9 the practice of passing NULL to gc-using memory
> allocation functions was introduced. However, the arrangements there
> were not correct as committed, because the error handling and logging
> depends on getting a ctx from the gc - so an allocation error would in
> fact result in libxl dereferencing NULL.
>
> Instead, provide a special dummy gc in the ctx, called `nogc_gc'. It
> is marked out specially by having alloc_maxsize==-1, which is
> otherwise invalid.
>
> Functions which need to actually look into the gc use the new test
> function gc_is_real (whose purpose is mainly clarity of the code) to
> check whether the gc is the dummy one, and do nothing if it is. And
> we provide a helper macro NOGC which uses the in-scope real gc to find
> the ctx and hence the dummy gc (and which replaces the previous
> #define NOGC NULL).
>
> Change all callers which pass 0 or NULL to an allocation function to
> use NOGC or &ctx->nogc_gc, as applicable in the context.
>
> We add a comment near the definition of LIBXL_INIT_GC pointing out
> that it isn't any more the only place a libxl__gc struct is
> initialised, for the benefit of anyone changing the contents of gc's
> in the future.
>
> Also, actually document that libxl__ptr_add is legal with ptr==NULL,
> and change a couple of calls not to check for NULL argument.
>
> Reported-by: Bamvor Jian Zhang <bjzhang [at] suse>
> Signed-off-by: Ian Jackson <ian.jackson [at] eu>
> Cc: Bamvor Jian Zhang <bjzhang [at] suse>
Acked-by: Ian Campbell <Ian.Campbell [at] citrix>
> ---
> tools/libxl/libxl.c | 3 +++
> tools/libxl/libxl_aoutils.c | 3 ++-
> tools/libxl/libxl_create.c | 2 +-
> tools/libxl/libxl_event.c | 5 +++--
> tools/libxl/libxl_exec.c | 2 +-
> tools/libxl/libxl_fork.c | 2 +-
> tools/libxl/libxl_internal.c | 11 +++++++++--
> tools/libxl/libxl_internal.h | 37 +++++++++++++++++++++++--------------
> tools/libxl/libxl_utils.c | 6 ++----
> tools/libxl/libxl_xshelp.c | 7 ++-----
> 10 files changed, 47 insertions(+), 31 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2a31528..a257902 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -41,6 +41,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>
> /* First initialise pointers etc. (cannot fail) */
>
> + ctx->nogc_gc.alloc_maxsize = -1;
> + ctx->nogc_gc.owner = ctx;
> +
> LIBXL_TAILQ_INIT(&ctx->occurred);
>
> ctx->osevent_hooks = 0;
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 7f8d6d3..99972a2 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -77,6 +77,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
> void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
> const void *data, size_t len)
> {
> + EGC_GC;
> libxl__datacopier_buf *buf;
> /*
> * It is safe for this to be called immediately after _start, as
> @@ -88,7 +89,7 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>
> assert(len < dc->maxsz - dc->used);
>
> - buf = libxl__zalloc(0, sizeof(*buf) - sizeof(buf->buf) + len);
> + buf = libxl__zalloc(NOGC, sizeof(*buf) - sizeof(buf->buf) + len);
> buf->used = len;
> memcpy(buf->buf, data, len);
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 00705d8..ab000bc 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -108,7 +108,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> }
>
> if (b_info->blkdev_start == NULL)
> - b_info->blkdev_start = libxl__strdup(0, "xvda");
> + b_info->blkdev_start = libxl__strdup(NOGC, "xvda");
>
> if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> if (!b_info->u.hvm.bios)
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 565d2c2..eb23a93 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -772,7 +772,7 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
> if (poller->fd_rindices_allocd < maxfd) {
> assert(ARRAY_SIZE_OK(poller->fd_rindices, maxfd));
> poller->fd_rindices =
> - libxl__realloc(0, poller->fd_rindices,
> + libxl__realloc(NOGC, poller->fd_rindices,
> maxfd * sizeof(*poller->fd_rindices));
> memset(poller->fd_rindices + poller->fd_rindices_allocd,
> 0,
> @@ -1099,9 +1099,10 @@ void libxl_event_free(libxl_ctx *ctx, libxl_event *event)
> libxl_event *libxl__event_new(libxl__egc *egc,
> libxl_event_type type, uint32_t domid)
> {
> + EGC_GC;
> libxl_event *ev;
>
> - ev = libxl__zalloc(0,sizeof(*ev));
> + ev = libxl__zalloc(NOGC,sizeof(*ev));
> ev->type = type;
> ev->domid = domid;
>
> diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
> index 082bf44..cfa379c 100644
> --- a/tools/libxl/libxl_exec.c
> +++ b/tools/libxl/libxl_exec.c
> @@ -280,7 +280,7 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
> int status, rc;
>
> libxl__spawn_init(ss);
> - ss->ssd = libxl__zalloc(0, sizeof(*ss->ssd));
> + ss->ssd = libxl__zalloc(NOGC, sizeof(*ss->ssd));
> libxl__ev_child_init(&ss->ssd->mid);
>
> rc = libxl__ev_time_register_rel(gc, &ss->timeout,
> diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
> index 9ff99e0..044ddad 100644
> --- a/tools/libxl/libxl_fork.c
> +++ b/tools/libxl/libxl_fork.c
> @@ -92,7 +92,7 @@ libxl__carefd *libxl__carefd_record(libxl_ctx *ctx, int fd)
> libxl__carefd *cf = 0;
>
> libxl_fd_set_cloexec(ctx, fd, 1);
> - cf = libxl__zalloc(NULL, sizeof(*cf));
> + cf = libxl__zalloc(&ctx->nogc_gc, sizeof(*cf));
> cf->fd = fd;
> LIBXL_LIST_INSERT_HEAD(&carefds, cf, entry);
> return cf;
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 8139520..fbff7d0 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -30,11 +30,16 @@ 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)
> + if (!gc_is_real(gc))
> return;
>
> if (!ptr)
> @@ -66,6 +71,8 @@ void libxl__free_all(libxl__gc *gc)
> void *ptr;
> int i;
>
> + assert(gc_is_real(gc));
> +
> for (i = 0; i < gc->alloc_maxsize; i++) {
> ptr = gc->alloc_ptrs[i];
> gc->alloc_ptrs[i] = NULL;
> @@ -104,7 +111,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 != NULL) {
> + } else if (new_ptr != ptr && 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 f90814a..e69482c 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -277,10 +277,18 @@ struct libxl__poller {
> int wakeup_pipe[2]; /* 0 means no fd allocated */
> };
>
> +struct libxl__gc {
> + /* mini-GC */
> + int alloc_maxsize; /* -1 means this is the dummy non-gc gc */
> + void **alloc_ptrs;
> + libxl_ctx *owner;
> +};
> +
> struct libxl__ctx {
> xentoollog_logger *lg;
> xc_interface *xch;
> struct xs_handle *xsh;
> + libxl__gc nogc_gc;
>
> const libxl_event_hooks *event_hooks;
> void *event_hooks_user;
> @@ -356,13 +364,6 @@ typedef struct {
>
> #define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))
>
> -struct libxl__gc {
> - /* mini-GC */
> - int alloc_maxsize;
> - void **alloc_ptrs;
> - libxl_ctx *owner;
> -};
> -
> struct libxl__egc {
> /* For event-generating functions only.
> * The egc and its gc may be accessed only on the creating thread. */
> @@ -420,6 +421,7 @@ struct libxl__ao {
> (gc).alloc_ptrs = 0; \
> (gc).owner = (ctx); \
> } while(0)
> + /* NB, also, a gc struct ctx->nogc_gc is initialised in libxl_ctx_alloc */
>
> static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
> {
> @@ -438,13 +440,20 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
> * All pointers returned by these functions are registered for garbage
> * collection on exit from the outermost libxl callframe.
> *
> - * However, where the argument is stated to be "gc_opt", NULL may be
> - * passed instead, in which case no garbage collection will occur; the
> - * pointer must later be freed with free(). This is for memory
> - * allocations of types (b) and (c).
> + * However, where the argument is stated to be "gc_opt", &ctx->nogc_gc
> + * may be passed instead, in which case no garbage collection will
> + * occur; the pointer must later be freed with free(). (Passing NULL
> + * for gc_opt is not permitted.) This is for memory allocations of
> + * types (b) and (c). The convenience macro NOGC should be used where
> + * possible.
> + *
> + * NOGC (and ctx->nogc_gc) may ONLY be used with functions which
> + * explicitly declare that it's OK. Use with nonconsenting functions
> + * may result in leaks of those functions' internal allocations on the
> + * psuedo-gc.
> */
> -/* register @ptr in @gc for free on exit from outermost libxl callframe. */
> -_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr);
> +/* register ptr in gc for free on exit from outermost libxl callframe. */
> +_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr /* may be NULL */);
> /* if this is the outermost libxl callframe then free all pointers in @gc */
> _hidden void libxl__free_all(libxl__gc *gc);
> /* allocate and zero @bytes. (similar to a gc'd malloc(3)+memzero()) */
> @@ -2110,7 +2119,7 @@ _hidden const char *libxl__device_model_savefile(libxl__gc *gc, uint32_t domid);
> #define GC_INIT(ctx) libxl__gc gc[1]; LIBXL_INIT_GC(gc[0],ctx)
> #define GC_FREE libxl__free_all(gc)
> #define CTX libxl__gc_owner(gc)
> -#define NOGC NULL
> +#define NOGC (&CTX->nogc_gc) /* pass only to consenting functions */
>
> /* Allocation macros all of which use the gc. */
>
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 67ef82c..08c7dac 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -58,8 +58,7 @@ char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
> char *libxl__domid_to_name(libxl__gc *gc, uint32_t domid)
> {
> char *s = libxl_domid_to_name(libxl__gc_owner(gc), domid);
> - if ( s )
> - libxl__ptr_add(gc, s);
> + libxl__ptr_add(gc, s);
> return s;
> }
>
> @@ -107,8 +106,7 @@ char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t poolid)
> char *libxl__cpupoolid_to_name(libxl__gc *gc, uint32_t poolid)
> {
> char *s = libxl_cpupoolid_to_name(libxl__gc_owner(gc), poolid);
> - if ( s )
> - libxl__ptr_add(gc, s);
> + libxl__ptr_add(gc, s);
> return s;
> }
>
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index 993f527..7fdf164 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -86,11 +86,8 @@ char * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char *path)
> char *ptr;
>
> ptr = xs_read(ctx->xsh, t, path, NULL);
> - if (ptr != NULL) {
> - libxl__ptr_add(gc, ptr);
> - return ptr;
> - }
> - return 0;
> + libxl__ptr_add(gc, ptr);
> + return ptr;
> }
>
> char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid)
_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel
|
