All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH v1 0/1] libxl/gentypes: add range init to array elements in json parsing
@ 2019-10-28 18:22 Oleksandr Grytsov
  2019-10-28 18:22 ` [Xen-devel] [PATCH v1 1/1] " Oleksandr Grytsov
  0 siblings, 1 reply; 17+ messages in thread
From: Oleksandr Grytsov @ 2019-10-28 18:22 UTC (permalink / raw)
  To: xen-devel
  Cc: anthony.perard, Oleksandr Grytsov, julien.grall, ian.jackson, wl

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

There are two old threads in which this issues was addresses ([1], [2]) but
the root cause wasn't defined. The real root cause is that when libxl creates
json file it doesn't put entries for fields with default value (which is
correct). Then during parsing json back to data, array elements are initialized
by zero in all cases. But they should be initialized to default value (if it is
not equals to zero).

[1] https://marc.info/?l=xen-devel&m=151378727210130&w=2
[2] https://marc.info/?l=xen-devel&m=157071551925734&w=2

Oleksandr Grytsov (1):
  libxl/gentypes: add range init to array elements in json parsing

 tools/libxl/gentypes.py | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.17.1


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing
  2019-10-28 18:22 [Xen-devel] [PATCH v1 0/1] libxl/gentypes: add range init to array elements in json parsing Oleksandr Grytsov
@ 2019-10-28 18:22 ` Oleksandr Grytsov
  2019-10-29 15:13   ` Julien Grall
  2019-10-29 15:19   ` [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing Ian Jackson
  0 siblings, 2 replies; 17+ messages in thread
From: Oleksandr Grytsov @ 2019-10-28 18:22 UTC (permalink / raw)
  To: xen-devel
  Cc: anthony.perard, Oleksandr Grytsov, julien.grall, ian.jackson, wl

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:
+            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"
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing
  2019-10-28 18:22 ` [Xen-devel] [PATCH v1 1/1] " Oleksandr Grytsov
@ 2019-10-29 15:13   ` Julien Grall
  2019-10-29 15:33     ` Ian Jackson
  2019-10-29 15:19   ` [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing Ian Jackson
  1 sibling, 1 reply; 17+ messages in thread
From: Julien Grall @ 2019-10-29 15:13 UTC (permalink / raw)
  To: Oleksandr Grytsov, xen-devel
  Cc: anthony.perard, ian.jackson, julien.grall, Oleksandr Grytsov, wl

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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing
  2019-10-28 18:22 ` [Xen-devel] [PATCH v1 1/1] " Oleksandr Grytsov
  2019-10-29 15:13   ` Julien Grall
@ 2019-10-29 15:19   ` Ian Jackson
  1 sibling, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:19 UTC (permalink / raw)
  To: Oleksandr Grytsov
  Cc: Jürgen Groß,
	wl, julien.grall, Oleksandr Grytsov, Anthony Perard, xen-devel

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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing
  2019-10-29 15:13   ` Julien Grall
@ 2019-10-29 15:33     ` Ian Jackson
  2019-10-29 15:46       ` Ian Jackson
  0 siblings, 1 reply; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:33 UTC (permalink / raw)
  To: Julien Grall
  Cc: wl, julien.grall, Oleksandr Grytsov, Oleksandr Grytsov,
	Ian Jackson, Anthony Perard, xen-devel

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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing
  2019-10-29 15:33     ` Ian Jackson
@ 2019-10-29 15:46       ` Ian Jackson
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 0/4] libxl: gentypes: initialise array elements in json Ian Jackson
                           ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:46 UTC (permalink / raw)
  To: Julien Grall, Oleksandr Grytsov, xen-devel, Anthony Perard,
	Oleksandr Grytsov, julien.grall, wl

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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Xen-devel] [XEN PATCH for-4.13 v2 0/4] libxl: gentypes: initialise array elements in json
  2019-10-29 15:46       ` Ian Jackson
@ 2019-10-29 15:54         ` Ian Jackson
  2019-10-29 16:09           ` Jürgen Groß
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 1/4] tools/libxl: gentypes.py: Prefer init_val to init_fn Ian Jackson
                           ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:54 UTC (permalink / raw)
  To: xen-devel; +Cc: Jürgen Groß, Ian Jackson

Oleksandr Grytsov discovered that the libxl json idl parsing fails to
properly initialise array elements.

Fixing this is not entirely straightforward, because the code to do
the initialisation is not conveniently available as a separate
function.

To ease debugging and review, I have broken this patch up into 4 very
small refactorings which do not change the output, plus the one patch
to call the newly-provided initialiser.

I think this version addresses comments from Julien Grall on earlier
versions.  IMO it is a candiate for 4.13, and also backporting.

Ian Jackson (3):
  tools/libxl: gentypes.py: Prefer init_val to init_fn
  libxl: gentypes.py: Break out field_pass in ..._copy_deprecated
  libxl: gentypes.py: Break out libxl_C_type_do_init

Oleksandr Grytsov (1):
  libxl: gentypes: initialise array elements in json

 tools/libxl/gentypes.py | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

-- 
2.11.0


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Xen-devel] [XEN PATCH for-4.13 v2 1/4] tools/libxl: gentypes.py: Prefer init_val to init_fn
  2019-10-29 15:46       ` Ian Jackson
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 0/4] libxl: gentypes: initialise array elements in json Ian Jackson
@ 2019-10-29 15:54         ` Ian Jackson
  2019-10-30 10:39           ` Anthony PERARD
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 2/4] libxl: gentypes.py: Break out field_pass in ..._copy_deprecated Ian Jackson
                           ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:54 UTC (permalink / raw)
  To: xen-devel; +Cc: Jürgen Groß, Ian Jackson

When both are provided, init_val is likely to be more direct.

No functional change with existing types: C output is identical.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/gentypes.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 6417c9dd8c..1769121468 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -311,10 +311,10 @@ def libxl_C_type_copy_deprecated(field, v, indent = "    ", vparent = None):
                                    field.type.pass_arg(v, vparent is None))
 
         s+= "    "
-        if field.type.init_fn is not None:
-            s+= "%s(%s);\n" % (field.type.init_fn, field_ptr)
-        elif field.type.init_val is not None:
+        if field.type.init_val is not None:
             s+= "%s = %s;\n" % (field_val, field.type.init_val)
+        elif field.type.init_fn is not None:
+            s+= "%s(%s);\n" % (field.type.init_fn, field_ptr)
         else:
             s+= "memset(%s, 0, sizeof(*%s));\n" % (field_ptr, field_ptr)
 
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Xen-devel] [XEN PATCH for-4.13 v2 2/4] libxl: gentypes.py: Break out field_pass in ..._copy_deprecated
  2019-10-29 15:46       ` Ian Jackson
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 0/4] libxl: gentypes: initialise array elements in json Ian Jackson
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 1/4] tools/libxl: gentypes.py: Prefer init_val to init_fn Ian Jackson
@ 2019-10-29 15:54         ` Ian Jackson
  2019-10-30 10:49           ` Anthony PERARD
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 3/4] libxl: gentypes.py: Break out libxl_C_type_do_init Ian Jackson
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json Ian Jackson
  4 siblings, 1 reply; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:54 UTC (permalink / raw)
  To: xen-devel; +Cc: Jürgen Groß, Ian Jackson

We are going to want this in a moment.

No functional change with existing types: C output is identical.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/gentypes.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 1769121468..62883acb2e 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -287,10 +287,10 @@ def libxl_C_type_copy_deprecated(field, v, indent = "    ", vparent = None):
         if field.type.check_default_fn is None:
             raise Exception(
 "Deprecated field %s type doesn't have a default value checker" % field.name)
-        field_val = field.type.pass_arg(v, vparent is None,
-                                        passby=idl.PASS_BY_VALUE)
-        field_ptr = field.type.pass_arg(v, vparent is None,
-                                        passby=idl.PASS_BY_REFERENCE)
+        field_pass = lambda by: field.type.pass_arg(v, vparent is None,
+                                                    passby=by)
+        field_val = field_pass(idl.PASS_BY_VALUE)
+        field_ptr = field_pass(idl.PASS_BY_REFERENCE)
         s+= "if (!%s(&p->%s) && !%s(%s))\n" % (field.type.check_default_fn,
                                                field.deprecated_by,
                                                field.type.check_default_fn,
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Xen-devel] [XEN PATCH for-4.13 v2 3/4] libxl: gentypes.py: Break out libxl_C_type_do_init
  2019-10-29 15:46       ` Ian Jackson
                           ` (2 preceding siblings ...)
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 2/4] libxl: gentypes.py: Break out field_pass in ..._copy_deprecated Ian Jackson
@ 2019-10-29 15:54         ` Ian Jackson
  2019-10-30 10:56           ` Anthony PERARD
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json Ian Jackson
  4 siblings, 1 reply; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:54 UTC (permalink / raw)
  To: xen-devel; +Cc: Jürgen Groß, Ian Jackson

This is going to be the common way to initialise things.
_libxl_C_type_init remains the thing for generating the body of the
init function, and for some special cases.

No functional change with existing types: C output is identical.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/gentypes.py | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 62883acb2e..124285cd66 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -165,6 +165,19 @@ def libxl_init_members(ty, nesting = 0):
     else:
         return []
     
+def libxl_C_type_do_init(ty, pass_arg, need_zero=True, indent="    "):
+    s=indent
+    if ty.init_val is not None:
+        s+= "%s = %s;\n" % (pass_arg(idl.PASS_BY_VALUE), ty.init_val)
+    elif ty.init_fn is not None:
+        s+= "%s(%s);\n" % (ty.init_fn, pass_arg(idl.PASS_BY_REFERENCE))
+    elif need_zero:
+        ptr = pass_arg(idl.PASS_BY_REFERENCE)
+        s+= "memset(%s, 0, sizeof(*%s));\n" % (ptr, ptr)
+    else:
+        s=""
+    return s
+
 def _libxl_C_type_init(ty, v, indent = "    ", parent = None, subinit=False):
     s = ""
     if isinstance(ty, idl.KeyedUnion):
@@ -309,15 +322,7 @@ def libxl_C_type_copy_deprecated(field, v, indent = "    ", vparent = None):
         if field.type.dispose_fn is not None:
             s+= "    %s(%s);\n" % (field.type.dispose_fn,
                                    field.type.pass_arg(v, vparent is None))
-
-        s+= "    "
-        if field.type.init_val is not None:
-            s+= "%s = %s;\n" % (field_val, field.type.init_val)
-        elif field.type.init_fn is not None:
-            s+= "%s(%s);\n" % (field.type.init_fn, field_ptr)
-        else:
-            s+= "memset(%s, 0, sizeof(*%s));\n" % (field_ptr, field_ptr)
-
+        s+=libxl_C_type_do_init(field.type, field_pass)
         s+= "}\n"
 
     if s != "":
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json
  2019-10-29 15:46       ` Ian Jackson
                           ` (3 preceding siblings ...)
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 3/4] libxl: gentypes.py: Break out libxl_C_type_do_init Ian Jackson
@ 2019-10-29 15:54         ` Ian Jackson
  2019-10-30 11:31           ` Anthony PERARD
  4 siblings, 1 reply; 17+ messages in thread
From: Ian Jackson @ 2019-10-29 15:54 UTC (permalink / raw)
  To: xen-devel; +Cc: Jürgen Groß, Ian Jackson, Oleksandr Grytsov

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Currently, array elements are initialized with calloc.  Which means
initialize all element fields with zero values.  If an entry is not
present in the json (which is entirely permitted), the element will be
all-bits-zero instead of the default value (which is wrong).

The fix is to initalise each array element before parsing it, using
the new libxl_C_type_do_init function.

With existing types this results in a lot of new calls like this:

      for (i=0; (t=libxl__json_array_get(x,i)); i++) {
 +            libxl_sched_params_init(&p->vcpus[i]);
              rc = libxl__sched_params_parse_json(gc, t, &p->vcpus[i]);

(indentation adjusted).  This looks right.  To check what happens with
types which have nontrivial defaults but don't have init functions (of
which we currently have none in arrays), I (Ian) experimentally added:

      ("pnode", uint32), # physical node of this node
      ("vcpus", libxl_bitmap), # vcpus in this node
 +    ("sporks", Array(MemKB, "num_sporks")),
      ])

The result was this:

          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]);

where the context was added by adding "sporks" and "+" indicates a
line added by this patch, "initialise array elements in json".

Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2 [iwj]: Use libxl_C_type_do_init.
          Reword commit message and discuss spork testing.
---
 tools/libxl/gentypes.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 124285cd66..c74445f16e 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -461,6 +461,10 @@ 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"
+        s += libxl_C_type_do_init(ty.elem_type,
+                    lambda(by): ("&" if by == idl.PASS_BY_REFERENCE else "")+
+                                ("%s[i]" % v),
+                                  need_zero=False, indent=indent+"    ")
         s += libxl_C_type_parse_json(ty.elem_type, "t", v+"[i]",
                                      indent + "    ", parent)
         s += "    }\n"
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [XEN PATCH for-4.13 v2 0/4] libxl: gentypes: initialise array elements in json
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 0/4] libxl: gentypes: initialise array elements in json Ian Jackson
@ 2019-10-29 16:09           ` Jürgen Groß
  0 siblings, 0 replies; 17+ messages in thread
From: Jürgen Groß @ 2019-10-29 16:09 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On 29.10.19 16:54, Ian Jackson wrote:
> Oleksandr Grytsov discovered that the libxl json idl parsing fails to
> properly initialise array elements.
> 
> Fixing this is not entirely straightforward, because the code to do
> the initialisation is not conveniently available as a separate
> function.
> 
> To ease debugging and review, I have broken this patch up into 4 very
> small refactorings which do not change the output, plus the one patch
> to call the newly-provided initialiser.
> 
> I think this version addresses comments from Julien Grall on earlier
> versions.  IMO it is a candiate for 4.13, and also backporting.
> 
> Ian Jackson (3):
>    tools/libxl: gentypes.py: Prefer init_val to init_fn
>    libxl: gentypes.py: Break out field_pass in ..._copy_deprecated
>    libxl: gentypes.py: Break out libxl_C_type_do_init
> 
> Oleksandr Grytsov (1):
>    libxl: gentypes: initialise array elements in json
> 
>   tools/libxl/gentypes.py | 35 ++++++++++++++++++++++-------------
>   1 file changed, 22 insertions(+), 13 deletions(-)
> 

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [XEN PATCH for-4.13 v2 1/4] tools/libxl: gentypes.py: Prefer init_val to init_fn
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 1/4] tools/libxl: gentypes.py: Prefer init_val to init_fn Ian Jackson
@ 2019-10-30 10:39           ` Anthony PERARD
  0 siblings, 0 replies; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 10:39 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Jürgen Groß, xen-devel

On Tue, Oct 29, 2019 at 03:54:33PM +0000, Ian Jackson wrote:
> When both are provided, init_val is likely to be more direct.
> 
> No functional change with existing types: C output is identical.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [XEN PATCH for-4.13 v2 2/4] libxl: gentypes.py: Break out field_pass in ..._copy_deprecated
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 2/4] libxl: gentypes.py: Break out field_pass in ..._copy_deprecated Ian Jackson
@ 2019-10-30 10:49           ` Anthony PERARD
  0 siblings, 0 replies; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 10:49 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Jürgen Groß, xen-devel

On Tue, Oct 29, 2019 at 03:54:34PM +0000, Ian Jackson wrote:
> We are going to want this in a moment.
> 
> No functional change with existing types: C output is identical.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [XEN PATCH for-4.13 v2 3/4] libxl: gentypes.py: Break out libxl_C_type_do_init
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 3/4] libxl: gentypes.py: Break out libxl_C_type_do_init Ian Jackson
@ 2019-10-30 10:56           ` Anthony PERARD
  0 siblings, 0 replies; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 10:56 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Jürgen Groß, xen-devel

On Tue, Oct 29, 2019 at 03:54:35PM +0000, Ian Jackson wrote:
> This is going to be the common way to initialise things.
> _libxl_C_type_init remains the thing for generating the body of the
> init function, and for some special cases.
> 
> No functional change with existing types: C output is identical.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json
  2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json Ian Jackson
@ 2019-10-30 11:31           ` Anthony PERARD
  2019-11-19 16:45             ` Ian Jackson
  0 siblings, 1 reply; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 11:31 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Jürgen Groß, xen-devel, Oleksandr Grytsov

On Tue, Oct 29, 2019 at 03:54:36PM +0000, Ian Jackson wrote:
> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> index 124285cd66..c74445f16e 100644
> --- a/tools/libxl/gentypes.py
> +++ b/tools/libxl/gentypes.py
> @@ -461,6 +461,10 @@ 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"
> +        s += libxl_C_type_do_init(ty.elem_type,
> +                    lambda(by): ("&" if by == idl.PASS_BY_REFERENCE else "")+

The syntax for using `lambda' is without "()" for the list of parameters.
python3 complains about it.

With that fix:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

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

Thanks,

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json
  2019-10-30 11:31           ` Anthony PERARD
@ 2019-11-19 16:45             ` Ian Jackson
  0 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2019-11-19 16:45 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: Jürgen Groß, xen-devel, Oleksandr Grytsov

Anthony PERARD writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json"):
> On Tue, Oct 29, 2019 at 03:54:36PM +0000, Ian Jackson wrote:
> > +                    lambda(by): ("&" if by == idl.PASS_BY_REFERENCE else "")+
> 
> The syntax for using `lambda' is without "()" for the list of parameters.
> python3 complains about it.

Oh.  Thanks.

> With that fix:
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks, fixed and pushed.

Ian.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-11-19 16:45 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-28 18:22 [Xen-devel] [PATCH v1 0/1] libxl/gentypes: add range init to array elements in json parsing Oleksandr Grytsov
2019-10-28 18:22 ` [Xen-devel] [PATCH v1 1/1] " Oleksandr Grytsov
2019-10-29 15:13   ` Julien Grall
2019-10-29 15:33     ` Ian Jackson
2019-10-29 15:46       ` Ian Jackson
2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 0/4] libxl: gentypes: initialise array elements in json Ian Jackson
2019-10-29 16:09           ` Jürgen Groß
2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 1/4] tools/libxl: gentypes.py: Prefer init_val to init_fn Ian Jackson
2019-10-30 10:39           ` Anthony PERARD
2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 2/4] libxl: gentypes.py: Break out field_pass in ..._copy_deprecated Ian Jackson
2019-10-30 10:49           ` Anthony PERARD
2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 3/4] libxl: gentypes.py: Break out libxl_C_type_do_init Ian Jackson
2019-10-30 10:56           ` Anthony PERARD
2019-10-29 15:54         ` [Xen-devel] [XEN PATCH for-4.13 v2 4/4] libxl: gentypes: initialise array elements in json Ian Jackson
2019-10-30 11:31           ` Anthony PERARD
2019-11-19 16:45             ` Ian Jackson
2019-10-29 15:19   ` [Xen-devel] [PATCH v1 1/1] libxl/gentypes: add range init to array elements in json parsing Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.