Re: [PATCH v2] binman: Skip node generation for images read from files

[Jan Kiszka <jan.kiszka@siemens.com>]

On 26.01.22 16:57, Simon Glass wrote:
> Hi Jan,
> 
> On Mon, 17 Jan 2022 at 00:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 forward the mode from the
>> image to every Entry object it contains 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.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Changes in v2:
>>    - more verbose commit log
>>
>>    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(-)
> 
> Did you miss my other comments and a test?
> 

Sorry, I indeed the other comments on the code - after ignoring the one 
about a test case. ENOTIME.

Jan

>>
>> 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
> 
> Regards,
> Simon

-- 
Siemens AG, Technology
Competence Center Embedded Linux