Re: [libvirt PATCH] esx: generator: fix free of elements in lists
On Thu, Sep 10, 2020 at 01:44:34PM +0200, Pino Toscano wrote: When a list is freed, we iterate through all the items, invoking the free function for each; the actual free function called for each element is the function of the actual type of each element, and thus the @_next pointer in the element struct has the same type as the element itself. Currently, the free function gets the parent of the current element type, and invoke its free function to continue freeing the list. However, in case the hierarchy of the classes has more than 1 level (i.e. Class <- SubClass <- SubSubClass), the invoked free function is only the parent class' one, and not the actual base class of the hierarchy. To fix that, change the generator to get the ancestor of a class, and invoking that instead. Also, avoid to set the @_next back, as it is not needed. I would say "base class" except "ancestor" because the naming confused me, but other than that it looks good to me. Reviewed-by: Martin Kletzander signature.asc Description: PGP signature
Re: [libvirt PATCH] esx: generator: fix free of elements in lists
On Thu, Sep 10, 2020 at 7:48 AM Pino Toscano wrote:
>
> When a list is freed, we iterate through all the items, invoking the
> free function for each; the actual free function called for each element
> is the function of the actual type of each element, and thus the @_next
> pointer in the element struct has the same type as the element itself.
> Currently, the free function gets the parent of the current element
> type, and invoke its free function to continue freeing the list.
> However, in case the hierarchy of the classes has more than 1 level
> (i.e. Class <- SubClass <- SubSubClass), the invoked free function is
> only the parent class' one, and not the actual base class of the
> hierarchy.
>
> To fix that, change the generator to get the ancestor of a class, and
> invoking that instead. Also, avoid to set the @_next back, as it is not
> needed.
>
> Fixes commits 5cff36e39ae691fbd7c40597df1732eecf294150 and
> f76c6dde2e33233566e886d96e76b5fe0c102d9a.
>
> Signed-off-by: Pino Toscano
> ---
> scripts/esx_vi_generator.py | 26 +-
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/esx_vi_generator.py b/scripts/esx_vi_generator.py
> index 863c8af964..be9b3949e8 100755
> --- a/scripts/esx_vi_generator.py
> +++ b/scripts/esx_vi_generator.py
> @@ -751,13 +751,13 @@ class Object(GenericObject):
> source += "{\n"
>
> if self.features & Object.FEATURE__LIST:
> -if self.extends is not None:
> +ancestor = get_ancestor(self)
> +if ancestor:
> # avoid "dereferencing type-punned pointer will break
> # strict-aliasing rules" warnings
> -source += "esxVI_%s *next = (esxVI_%s
> *)item->_next;\n\n" \
> - % (self.extends, self.extends)
> -source += "esxVI_%s_Free();\n" % self.extends
> -source += "item->_next = (esxVI_%s *)next;\n\n" %
> self.name
> +source += "esxVI_%s *baseNext = (esxVI_%s
> *)item->_next;\n" \
> + % (ancestor, ancestor)
> +source += "esxVI_%s_Free();\n\n" % ancestor
> else:
> source += "esxVI_%s_Free(>_next);\n\n" % self.name
>
> @@ -1250,6 +1250,21 @@ def is_known_type(type):
> type in enums_by_name)
>
>
> +def get_ancestor(obj):
> +if not obj.extends:
> +return None
> +ancestor = None
> +try:
> +ancestor = ancestor_by_name[obj.extends]
> +except KeyError:
> +parent = objects_by_name[obj.extends]
> +ancestor = get_ancestor(parent)
> +if not ancestor:
> +ancestor = parent.name
> +ancestor_by_name[name] = ancestor
> +return ancestor
> +
> +
> def open_file(filename):
> return open(filename, "wt")
>
> @@ -1341,6 +1356,7 @@ managed_objects_by_name = {}
> enums_by_name = {}
> methods_by_name = {}
> block = None
> +ancestor_by_name = {}
>
>
> # parse input file
> --
> 2.26.2
>
LGTM.
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
[libvirt PATCH] esx: generator: fix free of elements in lists
When a list is freed, we iterate through all the items, invoking the
free function for each; the actual free function called for each element
is the function of the actual type of each element, and thus the @_next
pointer in the element struct has the same type as the element itself.
Currently, the free function gets the parent of the current element
type, and invoke its free function to continue freeing the list.
However, in case the hierarchy of the classes has more than 1 level
(i.e. Class <- SubClass <- SubSubClass), the invoked free function is
only the parent class' one, and not the actual base class of the
hierarchy.
To fix that, change the generator to get the ancestor of a class, and
invoking that instead. Also, avoid to set the @_next back, as it is not
needed.
Fixes commits 5cff36e39ae691fbd7c40597df1732eecf294150 and
f76c6dde2e33233566e886d96e76b5fe0c102d9a.
Signed-off-by: Pino Toscano
---
scripts/esx_vi_generator.py | 26 +-
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/scripts/esx_vi_generator.py b/scripts/esx_vi_generator.py
index 863c8af964..be9b3949e8 100755
--- a/scripts/esx_vi_generator.py
+++ b/scripts/esx_vi_generator.py
@@ -751,13 +751,13 @@ class Object(GenericObject):
source += "{\n"
if self.features & Object.FEATURE__LIST:
-if self.extends is not None:
+ancestor = get_ancestor(self)
+if ancestor:
# avoid "dereferencing type-punned pointer will break
# strict-aliasing rules" warnings
-source += "esxVI_%s *next = (esxVI_%s *)item->_next;\n\n" \
- % (self.extends, self.extends)
-source += "esxVI_%s_Free();\n" % self.extends
-source += "item->_next = (esxVI_%s *)next;\n\n" % self.name
+source += "esxVI_%s *baseNext = (esxVI_%s
*)item->_next;\n" \
+ % (ancestor, ancestor)
+source += "esxVI_%s_Free();\n\n" % ancestor
else:
source += "esxVI_%s_Free(>_next);\n\n" % self.name
@@ -1250,6 +1250,21 @@ def is_known_type(type):
type in enums_by_name)
+def get_ancestor(obj):
+if not obj.extends:
+return None
+ancestor = None
+try:
+ancestor = ancestor_by_name[obj.extends]
+except KeyError:
+parent = objects_by_name[obj.extends]
+ancestor = get_ancestor(parent)
+if not ancestor:
+ancestor = parent.name
+ancestor_by_name[name] = ancestor
+return ancestor
+
+
def open_file(filename):
return open(filename, "wt")
@@ -1341,6 +1356,7 @@ managed_objects_by_name = {}
enums_by_name = {}
methods_by_name = {}
block = None
+ancestor_by_name = {}
# parse input file
--
2.26.2
