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

[Anthony PERARD <anthony.perard@citrix.com>]

On 09/25/2012 09:54 AM, Ian Campbell wrote:
> 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@citrix.com>
>> ---
>>  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);
>> +    }
>> +
>> +    arg = libxl__zalloc(NOGC, sizeof(*arg));
>> +
>> +    arg->map_key = libxl__strdup(NOGC, name);
>> +    arg->obj = obj;
>> +
>> +    flexarray_append((*param)->u.map, arg);
>> +}
>> +
>> +static void 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(NOGC, JSON_STRING);
>> +    obj->u.string = libxl__strdup(NOGC, argument);
>> +
>> +    qmp_parameters_common_add(gc, param, name, obj);
>> +}
>> +
>> +static void qmp_parameters_add_bool(libxl__gc *gc,
>> +                                    libxl__json_object **param,
>> +                                    const char *name, bool b)
>> +{
>> +    libxl__json_object *obj;
>> +
>> +    obj = libxl__json_object_alloc(NOGC, JSON_TRUE);
> 
> This is pre-existing, but treating JSON_TRUE and JSON_FALSE as distinct
> node types and not specific values of the (currently non-existent)
> JSON_BOOL type is a bit odd.

It was to save a bit of memory, and I probably follow another example
when I wrote those JSON_{TRUE,FALSE}.

I'll add another patch to introduce JSON_BOOL and remove those separated
true/false.

> I noticed it because you don't use the b param here... 

:(.

>> +    qmp_parameters_common_add(gc, param, name, obj);
>> +}
>> +
>> +#define QMP_PARAMETERS_SPRINTF(gc, args, name, format, ...) \
>> +    qmp_parameters_add_string(gc, args, name, \
>> +                              libxl__sprintf(gc, format, __VA_ARGS__))
> 
> I think it would be valid, and in keeping with the similar GC_SPRINTFOO
> macros, to make gc and perhaps args required in the calling environment
> rather than passing them, if you wanted to.

Seams fair to have gc required in the caller but I think I'll keep args
as a parameter of the macro.

Thanks,

-- 
Anthony PERARD