|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types
Please could you CC Anthony too, I think he wrote all this
libxl__json_object stuff.
On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> @@ -396,6 +396,66 @@ out:
> return s;
> }
>
> +int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
> + const libxl__json_object *o,
> + libxl_cpuid_policy_list *p)
> +{
> + int i, size;
> + libxl_cpuid_policy_list l;
> +
> + if (!libxl__json_object_is_array(o))
> + return -1;
> +
> + if (!o->u.array->count)
> + return 0;
> +
> + size = o->u.array->count;
> + /* need one extra slot as setinel */
"sentinel"
> + l = *p = calloc(size + 1, sizeof(libxl_cpuid_policy));
This function should GC_INIT and use the provided gc for allocations,
shouldn't it?.
> +
> + if (!l)
> + return -1;
> +
> + memset(l, 0, sizeof(libxl_cpuid_policy) * (size + 1));
> + l[size].input[0] = XEN_CPUID_INPUT_UNUSED;
> + l[size].input[1] = XEN_CPUID_INPUT_UNUSED;
> +
> + for (i = 0; i < size; i++) {
> + const libxl__json_object *t;
> + int j;
> +
> + if (flexarray_get(o->u.array, i, (void**)&t) != 0)
> + return -1; /* dispose_fn will clean up memory
> + * allocation */
You haven't done a gc allocation, so it won't. Looking closer I think
this falls into case #1 of the comment in libxl.h, which means the
allocation should have come from the heap, but that means you need to
manually free it.
(I see this comment about dispose_fn a lot, so I won't repeat it)
> + for (j = 0; j < 4; j++) {
This, and the code it is based on, really ought to use ARRAY_SIZE I
think.
> + const libxl__json_object *r;
> +
> + r = libxl__json_map_get(policy_names[j], t, JSON_STRING);
> + if (!r)
> + l[i].policy[j] = NULL;
> + else
> + l[i].policy[j] = strdup(r->u.string);
> + }
> + }
> +
> + assert(i == size);
Given a lack of break's in the for loop this seems overly defensive to
me.
> +
> + return 0;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index 6f1116f..e2d5dbe 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -100,6 +100,35 @@ yajl_gen_status libxl_defbool_gen_json(yajl_gen hand,
> return libxl__yajl_gen_asciiz(hand, libxl_defbool_to_string(*db));
> }
>
> +int libxl_defbool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> + libxl_defbool *p)
> +{
> + if (!libxl__json_object_is_string(o))
> + return -1;
> +
> + if (!strncmp(o->u.string, "<default>", strlen("<default>")))
> + p->val = 0;
> + else if (!strncmp(o->u.string, "True", strlen("True")))
> + p->val = 1;
> + else if (!strncmp(o->u.string, "False", strlen("False")))
Should have some internal #defines for these strings and use in the
generator too. Also you should use LIBXL__DEFBOOL_DEFAULT and friends.
> @@ -128,6 +166,34 @@ out:
> return s;
> }
>
> +int libxl_bitmap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> + libxl_bitmap *p)
> +{
> + int i;
> + int size;
> + const libxl__json_object *t;
> +
> + if (!libxl__json_object_is_array(o))
> + return -1;
> +
> + if (!o->u.array->count)
> + return 0;
> +
> + t = libxl__json_array_get(o, o->u.array->count - 1);
> + size = t->u.i + 1;
What is this doing? It's retrieving the last item in the array, assuming
it is an integer and then inferring the length of the array from it?
Oh this is because a bitmask is represented as an array which contains a
list of the bit indexes which are true.
Is the json array guaranteed to be sorted? I guess it is if libxl
generated it, but can we rely on that?
I wonder if it is too late to change the json representation of a
bitmask that we are using. Thoughts?
> +
> + libxl_bitmap_alloc(ctx, p, size);
> +
> + for (i = 0; (t = libxl__json_array_get(o, i)); i++) {
> + if (!libxl__json_object_is_integer(t))
> + return -1; /* dispose_fn will clean up memory
> + * allocation */
> + libxl_bitmap_set(p, t->u.i);
> + }
> +
> + return 0;
> +}
> +
> yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
> libxl_key_value_list *pkvl)
> {
> @@ -155,6 +221,43 @@ out:
> return s;
> }
>
> +int libxl_key_value_list_parse_json(libxl_ctx *ctx, const libxl__json_object
> *o,
> + libxl_key_value_list *p)
> +{
> + libxl__json_map_node *node = NULL;
> + flexarray_t *maps = NULL;
> + int i, size;
> + libxl_key_value_list kvl;
> +
> + if (!libxl__json_object_is_map(o))
> + return -1;
> +
> + maps = o->u.map;
> + size = maps->count * 2;
> + kvl = *p = calloc(size, sizeof(char *));
> + if (!kvl)
> + return -1;
> +
> + memset(kvl, 0, sizeof(char *) * size);
> +
> + for (i = 0; i < maps->count; i++) {
> + int idx = i * 2;
> + if (flexarray_get(maps, i, (void**)&node) != 0)
> + return -1;
> + assert(libxl__json_object_is_string(node->obj) ||
> + libxl__json_object_is_null(node->obj));
I think we may need to be mindful of potentially malicious json code
which a toolstack is feeding to these parser functions, don't we?
IOW asserting and crashing the entire daemon would be unfortunate.
> + kvl[idx] = strdup(node->map_key);
> + if (libxl__json_object_is_string(node->obj))
> + kvl[idx+1] = strdup(node->obj->u.string);
> + else
> + kvl[idx+1] = NULL;
> + }
> +
> + assert(i == maps->count);
Too defensive again.
> +
> + return 0;
> +}
> +
> yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list
> *pl)
> {
> libxl_string_list l = *pl;
> @@ -201,6 +343,27 @@ out:
> return s;
> }
>
> +int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> + libxl_hwcap *p)
> +{
> + int i;
> +
> + if (!libxl__json_object_is_array(o))
> + return -1;
I jut noticed the error handling here, but I'm sure it was the same for
all the preceding. libxl functions should return an ERROR_* code and not
-1 unless you have a good reason not to.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |