[PATCH RFC v2 5/8] binman: rework dropping absent entries from packaged image

[Yannic Moog]

Am Freitag, dem 30.05.2025 um 12:18 +0100 schrieb Simon Glass:
> Hi Yannic,
> 
> On Tue, 27 May 2025 at 14:24, Yannic Moog <y.moog at phytec.de> wrote:
> > 
> > When blobs are absent and are marked as optional, they can be safely
> > dropped from the binman tree. Use the drop_absent function for that.
> > Rename drop_absent to drop_absent_optional as we do not want to drop any
> > entries that are absent; they should be reported by binman as errors
> > when they are missing.
> > We also reorder the processing of the image the following:
> > - We call the CheckForProblems function before the image is built.
> > - We drop entries after we checked for problems with the image.
> > This is okay because CheckForProblems does not look at the file we have
> > written but rather queries the data structure (image) built with binman.
> > This also allows us to get all error and warning messages that we want
> > to report while avoiding putting missing optional entries in the final
> > image.
> > 
> > Signed-off-by: Yannic Moog <y.moog at phytec.de>
> > ---
> >  tools/binman/control.py       |  8 ++++----
> >  tools/binman/entry.py         |  6 +++++-
> >  tools/binman/etype/cbfs.py    |  3 ++-
> >  tools/binman/etype/mkimage.py |  2 +-
> >  tools/binman/etype/section.py | 18 +++++++++++++-----
> >  tools/binman/image.py         |  4 +++-
> >  6 files changed, 28 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tools/binman/control.py b/tools/binman/control.py
> > index
> > 81f61e3e152a9eab558cfc9667131a38082b61a1..c2a4d3eae9d4e8f4a3c5be1abfb1469ba1
> > c0ed93 100644
> > --- a/tools/binman/control.py
> > +++ b/tools/binman/control.py
> > @@ -18,6 +18,7 @@ import re
> > 
> >  import sys
> > 
> > +import image
> 
> This is done later in this file, to avoid warnings about missing
> modules when trying to get help ('binman -h'). I am guessing you need
> it for the type declaration below.
> 
> It may be that this is not necessary anymore though, in which case
> could you put this in a separate patch, use the 'from binman import'
> syntax and drop the other 'imports' from below?

Would like to leave this untouched to keep diff short after thinking about it.

> 
> >  from binman import bintool
> >  from binman import cbfs_util
> >  from binman import elf
> > @@ -669,7 +670,7 @@ def CheckForProblems(image):
> >                                     for bintool in missing_bintool_list])))
> >      return any([missing_list, faked_list, missing_bintool_list])
> > 
> > -def ProcessImage(image, update_fdt, write_map, get_contents=True,
> > +def ProcessImage(image: image.Image, update_fdt, write_map,
> > get_contents=True,
> >                   allow_resize=True, allow_missing=False,
> >                   allow_fake_blobs=False):
> >      """Perform all steps for this image, including checking and # writing
> > it.
> > @@ -697,7 +698,6 @@ def ProcessImage(image, update_fdt, write_map,
> > get_contents=True,
> >          image.SetAllowMissing(allow_missing)
> >          image.SetAllowFakeBlob(allow_fake_blobs)
> >          image.GetEntryContents()
> > -        image.drop_absent()
> >      image.GetEntryOffsets()
> > 
> >      # We need to pack the entries to figure out where everything
> > @@ -736,12 +736,12 @@ def ProcessImage(image, update_fdt, write_map,
> > get_contents=True,
> >          image.Raise('Entries changed size after packing (tried %s passes)'
> > %
> >                      passes)
> > 
> > +    has_problems = CheckForProblems(image)
> > +
> >      image.BuildImage()
> >      if write_map:
> >          image.WriteMap()
> > 
> > -    has_problems = CheckForProblems(image)
> > -
> >      image.WriteAlternates()
> > 
> >      return has_problems
> > diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> > index
> > 6390917e5083e40a4e3294e5d36ec300d2057fe9..ce7ef28e94b17e349544776070c457d588
> > 2661b1 100644
> > --- a/tools/binman/entry.py
> > +++ b/tools/binman/entry.py
> > @@ -760,7 +760,7 @@ class Entry(object):
> >                            self.image_pos)
> > 
> >      # pylint: disable=assignment-from-none
> > -    def GetEntries(self):
> > +    def GetEntries(self) -> None:
> >          """Return a list of entries contained by this entry
> > 
> >          Returns:
> > @@ -1351,6 +1351,10 @@ features to produce new behaviours.
> >              os.mkdir(cls.fake_dir)
> >          tout.notice(f"Fake-blob dir is '{cls.fake_dir}'")
> > 
> > +    def drop_absent_optional(self) -> None:
> > +        """Entries don't have any entries, do nothing"""
> > +        pass
> > +
> >      def ensure_props(self):
> >          """Raise an exception if properties are missing
> > 
> > diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
> > index
> > 886e34ef8221ad50e9f881e8abad16015c9790b5..bb97b227ef78356b08b5cc31b04d98c6fa
> > 0633ef 100644
> > --- a/tools/binman/etype/cbfs.py
> > +++ b/tools/binman/etype/cbfs.py
> > @@ -276,7 +276,8 @@ class Entry_cbfs(Entry):
> >          for entry in self.GetEntries().values():
> >              entry.ListEntries(entries, indent + 1)
> > 
> > -    def GetEntries(self):
> > +    def GetEntries(self) -> dict[str, Entry]:
> > +        """Returns the entries (tree children) of this section"""
> >          return self._entries
> > 
> >      def ReadData(self, decomp=True, alt_format=None):
> > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> > index
> > 19ee30a13ca75a5d05a17d4c26424e84934b6ea2..c9362c6bbf740da946abe2fadefe35ebfc
> > 89fced 100644
> > --- a/tools/binman/etype/mkimage.py
> > +++ b/tools/binman/etype/mkimage.py
> > @@ -205,7 +205,7 @@ class Entry_mkimage(Entry_section):
> >              self.record_missing_bintool(self.mkimage)
> >              return data
> > 
> > -    def GetEntries(self):
> > +    def GetEntries(self) -> dict[str, Entry]:
> >          # Make a copy so we don't change the original
> >          entries = OrderedDict(self._entries)
> >          if self._imagename:
> > diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> > index
> > 44b1b85e93496d0ca114907f42a98da1f0af09b9..8e5c20e3017c9b320ebfdfd3bf82489ed6
> > 161c98 100644
> > --- a/tools/binman/etype/section.py
> > +++ b/tools/binman/etype/section.py
> > @@ -450,7 +450,7 @@ class Entry_section(Entry):
> >      def _PackEntries(self):
> >          """Pack all entries into the section"""
> >          offset = self._skip_at_start
> > -        for entry in self._entries.values():
> > +        for entry in self.GetEntries().values():
> 
> As per previous patch, this should not be needed.

Agree.


> 
> >              offset = entry.Pack(offset)
> >          return offset
> > 
> > @@ -533,7 +533,7 @@ class Entry_section(Entry):
> >          for entry in self.GetEntries().values():
> >              entry.WriteMap(fd, indent + 1)
> > 
> > -    def GetEntries(self):
> > +    def GetEntries(self) -> dict[str, Entry]:
> >          return self._entries
> > 
> >      def GetContentsByPhandle(self, phandle, source_entry, required):
> > @@ -768,9 +768,17 @@ class Entry_section(Entry):
> >                         todo)
> >          return True
> > 
> > -    def drop_absent(self):
> > -        """Drop entries which are absent"""
> > -        self._entries = {n: e for n, e in self._entries.items() if not
> > e.absent}
> > +    def drop_absent_optional(self) -> None:
> > +        """Drop entries which are absent.
> > +        Call for all nodes in the tree. Leaf nodes will do nothing per
> > +        definition. Sections however have _entries and should drop all
> > children
> 
> should this be: all optional children ?

Yes, forgot to update, thanks.

> 
> > +        which are absent.
> > +        """
> > +        self._entries = {n: e for n, e in self._entries.items() if not
> > (e.absent and e.optional)}
> > +        # Drop nodes first before traversing children to avoid superfluous
> > calls
> > +        # to children of absent nodes.
> > +        for e in self.GetEntries().values():
> 
> As above, I think self._entries is correct
> 
> > +            e.drop_absent_optional()
> > 
> >      def _SetEntryOffsetSize(self, name, offset, size):
> >          """Set the offset and size of an entry
> > diff --git a/tools/binman/image.py b/tools/binman/image.py
> > index
> > 24ce0af7c72b5256a9a71963a6d94c080ed8bdd4..86543ad8db36b38afc1957fde7e2015206
> > 7b79aa 100644
> > --- a/tools/binman/image.py
> > +++ b/tools/binman/image.py
> > @@ -15,7 +15,7 @@ import sys
> >  from binman.entry import Entry
> >  from binman.etype import fdtmap
> >  from binman.etype import image_header
> > -from binman.etype import section
> > +from etype import section
> 
> Why this change? I suspect it will break when installed with 'pip
> install binary-manager'

I only used this to get lsp working, will drop this change.


> 
> >  from dtoc import fdt
> >  from dtoc import fdt_util
> >  from u_boot_pylib import tools
> > @@ -183,6 +183,8 @@ class Image(section.Entry_section):
> >          fname = tools.get_output_filename(self._filename)
> >          tout.info("Writing image to '%s'" % fname)
> >          with open(fname, 'wb') as fd:
> > +            # For final image, don't write absent blobs to file
> > +            self.drop_absent_optional()
> >              data = self.GetPaddedData()
> >              fd.write(data)
> >          tout.info("Wrote %#x bytes" % len(data))
> > 
> > --
> > 2.43.0
> > 
> 
> Regards,
> Simon