Re: [Xen-devel] [PATCH 3/6] libxl_qmp: Introduces helpers to create an argument list.

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Anthony PERARD writes ("[PATCH 3/6] libxl_qmp: Introduces helpers to create an 
argument list."):
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index e33b130..edbd3b4 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -623,6 +623,92 @@ static void qmp_free_handler(libxl__qmp_handler *qmp)
>      free(qmp);
>  }
>  
> +#if 0
> +/*
> + * QMP Parameters Helpers
> + * If a function fail to allocate memomy for one more parameters, then
> + * the param is free, even if it's come from a caller.

Allocation failures are supposed to be fatal nowadays, so this case
doesn't need to exit.

> +static libxl__json_object *qmp_parameters_common(libxl__gc *gc,
> +                                                 libxl__json_object *param,
> +                                                 const char *name,
> +                                                 libxl__json_object *obj)
> +{
> +    // check that every caller of this free the tree !

We use /* ... */.  I'm not sure what the purpose of this comment is.
Do you mean "this is a fixme" ?  Or is that intended to be a doc
comment saying that callers end up owning some object ?  (The return
value?)  Why is this object not from the gc ?

> +    arg = calloc(1, sizeof (libxl__json_map_node));

These allcations should all use the appropriate libxl helpers,
probably.  (Throughout all your patches.)

I'm not sure exactly what this function _does_, though.  It prepares
some "common" parameters for a qmp call, evidently, but what common
parameters ?

OTOH the code seems to add a single parameter to the map ?

> +static libxl__json_object *qmp_parameters_add_string(libxl__gc *gc,
> +                                                     libxl__json_object 
> *param,
> +                                                     const char *name,
> +                                                     const char *argument)
> +{
> +    libxl__json_object *obj;
> +
> +    obj = libxl__json_object_alloc(gc, JSON_STRING);
> +    if (!obj)
> +        goto error_nomem;
> +    obj->u.string = strdup(argument);
> +    if (!obj->u.string)
> +        goto error_nomem;
> +
> +    param = qmp_parameters_common(gc, param, name, obj);
> +    if (!param)
> +        goto error_nomem;
> +
> +    return param;
> +error_nomem:
> +    libxl__json_object_free(gc, param);
> +    libxl__json_object_free(gc, obj);
> +    return NULL;
> +}

This is very similar to the same function for bool.  Perhaps it would
be better to separate out the type-specific object creation ?

Ian.

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