| Message ID | 20191028182216.3882-2-al1img@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl/gentypes: add range init to array elements in json parsing | expand |
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@epam.com> > > 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@epam.com> > --- > 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,
Oleksandr Grytsov writes ("[PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>
> 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.
Julien Grall writes ("Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing"):
> 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.
Yes.
> I will repeat them here for convenience.
Thanks. It looks like our mails about this patch crossed.
> 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.
Yes.
> I am also not entirely sure whether we should also cater the
> ty.init_val != None as well here.
We should.
I have a revised patch. It makes no difference to the C output,
compared to Oleksandr's patch. I assume we have no arrays of things
with an init_val...
Ian.
Ian Jackson writes ("Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing"):
> Julien Grall writes ("Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing"):
> > I am also not entirely sure whether we should also cater the
> > ty.init_val != None as well here.
>
> We should.
>
> I have a revised patch. It makes no difference to the C output,
> compared to Oleksandr's patch. I assume we have no arrays of things
> with an init_val...
I experimentally added this:
modified tools/libxl/libxl_types.idl
@@ -461,6 +461,7 @@ libxl_vnode_info = Struct("vnode_info", [
("distances", Array(uint32, "num_distances")), # distances from this node to other nodes
("pnode", uint32), # physical node of this node
("vcpus", libxl_bitmap), # vcpus in this node
+ ("sporks", Array(MemKB, "num_sporks")),
])
libxl_gic_version = Enumeration("gic_version", [
This generates code containing this, to do json parsing of the sporks
array:
@@ -12657,6 +12657,7 @@
goto out;
}
for (i=0; (t=libxl__json_array_get(x,i)); i++) {
+ p->sporks[i] = LIBXL_MEMKB_DEFAULT;
rc = libxl__uint64_parse_json(gc, t, &p->sporks[i]);
if (rc)
goto out;
Here "+" is a line which is missing from the output of Oleksandr's
version and present in the output of mine. I think this means I have
convinced myself that we correctly identified a latent bug here and
that I have fixed it.
I will send out a revised version of this series shortly.
I think it is a candidate for 4.13.
Thanks,
Ian.
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: + s += indent + " "+"%s_init(&%s[i]);\n" % (ty.elem_type.typename, v) s += libxl_C_type_parse_json(ty.elem_type, "t", v+"[i]", indent + " ", parent) s += " }\n"