Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Oleksandr Grytsov writes ("[PATCH v1 1/1] libxl/gentypes: add range init to 
array elements in json parsing"):
> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
> 
> Add initialization of array elements in parse json function.
> 
> Currently, array elements are initialized with calloc. Which means
> initialize all element fields with zero values. If entries are missed in
> json for such fields, it will be equal to zero instead of default values.
> The fix is to add range init function before parsing array element for
> structures which have defined range init function.

I think you have accurately identified a bug.  Thank you.
I have eyeballed the diff to the output and it looks plausible as far
as it goes.

However,

>          s += "    for (i=0; (t=libxl__json_array_get(x,i)); i++) {\n"
> +        if ty.elem_type.init_fn is not None and 
> ty.elem_type.autogenerate_init_fn:
> +            s += indent + "    "+"%s_init(&%s[i]);\n" % 
> (ty.elem_type.typename, v)
>          s += libxl_C_type_parse_json(ty.elem_type, "t", v+"[i]",

I think open-coding the use of init_fn is wrong here.  I worry that
the effect would be to fail to initialise some things: in particular,
things with an init_val but no init_fn.

Looking at other places where init_fn is used:

 * _libxl_C_type_init is used for generating the body of %s_init.
   Using the output of that would obviously be logically correct here,
   but it's probably undesirable because it would emit a repetition of
   the per-field initialisers for aggregates.  It contains code which
   tries various strategies for initialisation.

 * libxl_C_type_member_init is a special case for typed unions, which
   if we get things right we shouldn't need to explicitly special-case
   here.

 * libxl_C_type_copy_deprecated also has code which tries various
   strategies for initialisation.

 * The other places are just .h and other similar bureaucracy.

I think therefore that the code in _libxl_C_type_init or in
libxl_C_type_copy_deprecated, or something like those, must be the
model.

Aggregates, including Struct and KeyedUnion, all have init_fn.  (I
think the "or ty.init_fn is None" at gentypes.py:197 is never true.)
For all aggregates, we want to call the function.  So in that respect,
libxl_C_type_copy_deprecated is more correct.

For non-aggregates which have a plain value (init_val), we would
prefer to set the value, as that is probably smaller code and faster
too.  But I think this is true for libxl_C_type_copy_deprecated too.

So I think the right code is something like that in
libxl_C_type_copy_deprecated, but with the init_val check first.

Ideally we would change libxl_C_type_copy_deprecated too.

I think I will try having a go at this myself.  Watch this space.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel