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

[Julien Grall <julien@xxxxxxx>]

Hi,

I have made some comments regarding the patch in the original thread. While I am 
not a libxl expert, it would have been nice to address them or at least explain 
why they weren't addressed.
I will repeat them here for convenience.

On 28/10/2019 18:22, Oleksandr Grytsov wrote:
> 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.
>
> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
> ---
>   tools/libxl/gentypes.py | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> index 6417c9dd8c..4ff5d8a2d0 100644
> --- a/tools/libxl/gentypes.py
> +++ b/tools/libxl/gentypes.py
> @@ -456,6 +456,8 @@ def libxl_C_type_parse_json(ty, w, v, indent = "    ", 
> parent = None, discrimina
>           s += "        goto out;\n"
>           s += "    }\n"
>           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:
My knowledge of libxl is quite limited. But I don't think this is correct, you
want to call init_fn whether this has been autogenerated or not.

> +            s += indent + "    "+"%s_init(&%s[i]);\n" % 
> (ty.elem_type.typename, v)
Looking at the other usage (like _libxl_C_type_init), init_fn is called with

             s += "%s(%s);\n" % (ty.init_fn, ty.pass_arg(v, parent is None))

I am also not entirely sure whether we should also cater the ty.init_val != None
as well here.

>           s += libxl_C_type_parse_json(ty.elem_type, "t", v+"[i]",
>                                        indent + "    ", parent)
>           s += "    }\n"
Cheers,

--
Julien Grall

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