Re: [PATCH] binman: Skip node generation for images read from files
Hi Jan, On Fri, 28 Jan 2022 at 12:37, Jan Kiszka wrote: > > From: Jan Kiszka > > We can and should run the node generator only when creating a new image. > When we read it back, there is no need to generate nodes - they already > exits, and binman does not dive that deep into the image - and there is > no way to provide the required fdt-list. So store the mode in the image > object so that Entry_fit can simply skip generator nodes when reading > them from an fdtmap. > > This unbreaks all read-backs of images that contain generator nodes in > their fdtmap. To confirm this, add a corresponding test case. > > Signed-off-by: Jan Kiszka > --- > tools/binman/etype/fit.py | 2 +- > tools/binman/ftest.py | 18 ++ > tools/binman/image.py | 9 +++-- > tools/binman/test/219_fit_gennode.dts | 24 > 4 files changed, 50 insertions(+), 3 deletions(-) > create mode 100644 tools/binman/test/219_fit_gennode.dts (version 3 patch) Reviewed-by: Simon Glass Tested-by: Simon Glass This has a lot of merge conflicts but they are my fault, so I am going to tidy them up. PLMK if something looks wrong. Regards, Simon
[PATCH] binman: Skip node generation for images read from files
From: Jan Kiszka
We can and should run the node generator only when creating a new image.
When we read it back, there is no need to generate nodes - they already
exits, and binman does not dive that deep into the image - and there is
no way to provide the required fdt-list. So store the mode in the image
object so that Entry_fit can simply skip generator nodes when reading
them from an fdtmap.
This unbreaks all read-backs of images that contain generator nodes in
their fdtmap. To confirm this, add a corresponding test case.
Signed-off-by: Jan Kiszka
---
tools/binman/etype/fit.py | 2 +-
tools/binman/ftest.py | 18 ++
tools/binman/image.py | 9 +++--
tools/binman/test/219_fit_gennode.dts | 24
4 files changed, 50 insertions(+), 3 deletions(-)
create mode 100644 tools/binman/test/219_fit_gennode.dts
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 6e5f020c502..6ad4a686df5 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -194,7 +194,7 @@ class Entry_fit(Entry):
# the FIT (e.g. "/images/kernel/u-boot"), so don't call
# fsw.add_node() or _AddNode() for it.
pass
-elif subnode.name.startswith('@'):
+elif self.GetImage().generate and subnode.name.startswith('@'):
if self._fdts:
# Generate notes for each FDT
for seq, fdt_fname in enumerate(self._fdts):
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index ca200ae9f8f..5400f76c676 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5100,6 +5100,24 @@ fdt fdtmapExtract the devicetree
blob from the fdtmap
self.assertIn('Documentation is missing for modules: mkimage',
str(e.exception))
+def testListWithGenNode(self):
+"""Check handling of an FDT map when the section cannot be found"""
+entry_args = {
+'of-list': 'test-fdt1 test-fdt2',
+}
+data = self._DoReadFileDtb(
+'219_fit_gennode.dts',
+entry_args=entry_args,
+use_real_dtb=True,
+extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])
+
+try:
+tmpdir, updated_fname = self._SetupImageInTmpdir()
+with test_util.capture_sys_output() as (stdout, stderr):
+self._RunBinman('ls', '-i', updated_fname)
+finally:
+shutil.rmtree(tmpdir)
+
if __name__ == "__main__":
unittest.main()
diff --git a/tools/binman/image.py b/tools/binman/image.py
index 0f0c1d29e80..cb5279c7ead 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -67,9 +67,13 @@ class Image(section.Entry_section):
does not exist in binman. This is useful if an image was created by
binman a newer version of binman but we want to list it in an older
version which does not support all the entry types.
+generate: If true, generator nodes are processed. If false they are
+ignored which is useful when an existing image is read back from a
+file.
"""
def __init__(self, name, node, copy_to_orig=True, test=False,
- ignore_missing=False, use_expanded=False,
missing_etype=False):
+ ignore_missing=False, use_expanded=False, missing_etype=False,
+ generate=True):
super().__init__(None, 'section', node, test=test)
self.copy_to_orig = copy_to_orig
self.name = 'main-section'
@@ -83,6 +87,7 @@ class Image(section.Entry_section):
self.use_expanded = use_expanded
self.test_section_timeout = False
self.bintools = {}
+self.generate = generate
if not test:
self.ReadNode()
@@ -131,7 +136,7 @@ class Image(section.Entry_section):
# Return an Image with the associated nodes
root = dtb.GetRoot()
image = Image('image', root, copy_to_orig=False, ignore_missing=True,
- missing_etype=True)
+ missing_etype=True, generate=False)
image.image_node = fdt_util.GetString(root, 'image-node', 'image')
image.fdtmap_dtb = dtb
diff --git a/tools/binman/test/219_fit_gennode.dts
b/tools/binman/test/219_fit_gennode.dts
new file mode 100644
index 000..7820c472808
--- /dev/null
+++ b/tools/binman/test/219_fit_gennode.dts
@@ -0,0 +1,24 @@
+/dts-v1/;
+
+/ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ binman {
+ fit {
+ description = "test-desc";
+ #address-cells = <1>;
+ fit,fdt-list = "of-list";
+
+ images {
+ @fdt-SEQ {
+ description = "
Re: [PATCH] binman: Skip node generation for images read from files
Hi Jan,
On Sun, 16 Jan 2022 at 08:51, Jan Kiszka wrote:
>
> From: Jan Kiszka
>
> This unbreaks all read-backs of images that contain generator nodes in
> their fdtmap.
This issue is subtle enough that I think it could use a few lines of
explanation.
>
> Signed-off-by: Jan Kiszka
> ---
>
> I tried to write some test case as well, but the testsuite is too
> fragile and too non-intuitive to me to extend it. E.g., just adding a
> fdtmap to 170_fit_fdt.dts made existing tests fail, for unclear reasons.
> I have to leave that to someone who understands the mechanics better.
That's because a fake FDT is used in most tests, to save time and
reduce complexity. In your case you need a real one so that you don't
just get fake junk in the fdtmap. The -9 FDT_ERR_BADMAGIC is produced
because the fdt is not really an fdt, but is U_BOOT_DTB_DATA (i.e.
'udtb').
You can call _DoReadFileRealDtb() to make things work - see testFdpmap().
But note that you should not reuse an existing dts for new tests.
Create a new one with the things you want in it and use that in your
new test.
>
> tools/binman/entry.py | 5 -
> tools/binman/etype/fit.py | 2 +-
> tools/binman/etype/section.py | 4 ++--
> tools/binman/image.py | 7 ---
> 4 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index bac90bbbcd..fdb9746fda 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -75,7 +75,7 @@ class Entry(object):
> available. This is mainly used for testing.
> external: True if this entry contains an external binary blob
> """
> -def __init__(self, section, etype, node, name_prefix=''):
> +def __init__(self, section, etype, node, name_prefix='', generate=None):
> # Put this here to allow entry-docs and help to work without libfdt
> global state
> from binman import state
> @@ -105,6 +105,9 @@ class Entry(object):
> self.external = False
> self.allow_missing = False
> self.allow_fake = False
> +if generate == None:
is None
> +generate = section.generate if section else True
> +self.generate = generate
But I think it would be simpler to have a flag in the Image (as you
have) and not copy it elsewhere.
>
> @staticmethod
> def FindEntryClass(etype, expanded):
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index b41187df80..4e4d2f9c22 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -193,7 +193,7 @@ class Entry_fit(Entry):
> # the FIT (e.g. "/images/kernel/u-boot"), so don't call
> # fsw.add_node() or _AddNode() for it.
> pass
> -elif subnode.name.startswith('@'):
> +elif self.generate and subnode.name.startswith('@'):
You can call self.GetImage().generate here so you don't need to copy
the 'generate' flag.
> if self._fdts:
> # Generate notes for each FDT
> for seq, fdt_fname in enumerate(self._fdts):
> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index 7a55d03231..319156a09a 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -154,9 +154,9 @@ class Entry_section(Entry):
> available. This is set by the `SetAllowMissing()` method, if
> `--allow-missing` is passed to binman.
> """
> -def __init__(self, section, etype, node, test=False):
> +def __init__(self, section, etype, node, test=False, generate=None):
> if not test:
> -super().__init__(section, etype, node)
> +super().__init__(section, etype, node, generate=generate)
> self._entries = OrderedDict()
> self._pad_byte = 0
> self._sort = False
> diff --git a/tools/binman/image.py b/tools/binman/image.py
> index f0a7d65299..1ff97e687c 100644
> --- a/tools/binman/image.py
> +++ b/tools/binman/image.py
> @@ -69,8 +69,9 @@ class Image(section.Entry_section):
> version which does not support all the entry types.
> """
> def __init__(self, name, node, copy_to_orig=True, test=False,
> - ignore_missing=False, use_expanded=False,
> missing_etype=False):
> -super().__init__(None, 'section', node, test=test)
> + ignore_missing=False, use_expanded=False,
> missing_etype=False,
> + generate=True):
Please remember to document 'generate' in the comments for class Image.
> +super().__init__(None, 'section', node, test=test, generate=generate)
> self.copy_to_orig = copy_to_orig
> self.name = 'main-section'
> self.image_name = name
> @@ -130,7 +131,7 @@ class Image(section.Entry_section):
> # Return an Image with the associated nodes
> roo
[PATCH] binman: Skip node generation for images read from files
From: Jan Kiszka
This unbreaks all read-backs of images that contain generator nodes in
their fdtmap.
Signed-off-by: Jan Kiszka
---
I tried to write some test case as well, but the testsuite is too
fragile and too non-intuitive to me to extend it. E.g., just adding a
fdtmap to 170_fit_fdt.dts made existing tests fail, for unclear reasons.
I have to leave that to someone who understands the mechanics better.
tools/binman/entry.py | 5 -
tools/binman/etype/fit.py | 2 +-
tools/binman/etype/section.py | 4 ++--
tools/binman/image.py | 7 ---
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index bac90bbbcd..fdb9746fda 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -75,7 +75,7 @@ class Entry(object):
available. This is mainly used for testing.
external: True if this entry contains an external binary blob
"""
-def __init__(self, section, etype, node, name_prefix=''):
+def __init__(self, section, etype, node, name_prefix='', generate=None):
# Put this here to allow entry-docs and help to work without libfdt
global state
from binman import state
@@ -105,6 +105,9 @@ class Entry(object):
self.external = False
self.allow_missing = False
self.allow_fake = False
+if generate == None:
+generate = section.generate if section else True
+self.generate = generate
@staticmethod
def FindEntryClass(etype, expanded):
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index b41187df80..4e4d2f9c22 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -193,7 +193,7 @@ class Entry_fit(Entry):
# the FIT (e.g. "/images/kernel/u-boot"), so don't call
# fsw.add_node() or _AddNode() for it.
pass
-elif subnode.name.startswith('@'):
+elif self.generate and subnode.name.startswith('@'):
if self._fdts:
# Generate notes for each FDT
for seq, fdt_fname in enumerate(self._fdts):
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 7a55d03231..319156a09a 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -154,9 +154,9 @@ class Entry_section(Entry):
available. This is set by the `SetAllowMissing()` method, if
`--allow-missing` is passed to binman.
"""
-def __init__(self, section, etype, node, test=False):
+def __init__(self, section, etype, node, test=False, generate=None):
if not test:
-super().__init__(section, etype, node)
+super().__init__(section, etype, node, generate=generate)
self._entries = OrderedDict()
self._pad_byte = 0
self._sort = False
diff --git a/tools/binman/image.py b/tools/binman/image.py
index f0a7d65299..1ff97e687c 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -69,8 +69,9 @@ class Image(section.Entry_section):
version which does not support all the entry types.
"""
def __init__(self, name, node, copy_to_orig=True, test=False,
- ignore_missing=False, use_expanded=False,
missing_etype=False):
-super().__init__(None, 'section', node, test=test)
+ ignore_missing=False, use_expanded=False, missing_etype=False,
+ generate=True):
+super().__init__(None, 'section', node, test=test, generate=generate)
self.copy_to_orig = copy_to_orig
self.name = 'main-section'
self.image_name = name
@@ -130,7 +131,7 @@ class Image(section.Entry_section):
# Return an Image with the associated nodes
root = dtb.GetRoot()
image = Image('image', root, copy_to_orig=False, ignore_missing=True,
- missing_etype=True)
+ missing_etype=True, generate=False)
image.image_node = fdt_util.GetString(root, 'image-node', 'image')
image.fdtmap_dtb = dtb
--
2.31.1
