Re: [Xen-devel] [PATCH V2 4/9] libxl_qmp: Introduces helpers to create an argument list.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:
> Those functions will be used to create a "list" of parameters that contain 
> more
> than just strings. This list is converted by qmp_send to a string to be sent 
> to
> QEMU.
> 
> Those functions will be used in the next two patches, so right now there are
> not compiled.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_qmp.c | 53 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 07a8bd5..1dd5c6c 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -623,6 +623,59 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
>      free(qmp);
>  }
>  
> +#if 0
> +/*
> + * QMP Parameters Helpers
> + * Those functions does not use the gc, because of the internal usage of
> + * flexarray that does not support it.
> + * The allocated *param need to be free with libxl__json_object_free(gc, 
> param)
> + */
> +static void qmp_parameters_common_add(libxl__gc *gc,
> +                                      libxl__json_object **param,
> +                                      const char *name,
> +                                      libxl__json_object *obj)
> +{
> +    libxl__json_map_node *arg = NULL;
> +
> +    if (!*param) {
> +        *param = libxl__json_object_alloc(NOGC, JSON_MAP);
> +    }

Having now read the callers in later patches this looks a bit odd to me
since they do

        args = NULL

        qmp_param.._add_foo(gc, args,...)

        qmp_run_thing

        libxl__json_object_free(...args...)

which is unbalanced WRT allocation and free and I can easily imagine
people forgetting the free.

If you can't arrange for these object's to be gc'd then I think having
an explicit alloc in the caller would be less liable to surprise people.

#define qmp_args_alloc(gc) libxl__json_object_alloc(NOGC, JSON_MAP)
#define qmp_args_free(..) libxl__json_object_free(...args...)

would work nicely IMHO.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel