[PATCH RFC v2 3/8] tools: binman: mark optional missing blobs as absent

[Yannic Moog]

Hi Simon,

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:
> > 
> > Optional blobs should mark themselves as absent to avoid being packed
> > into an image.
> > Extend the documentation of this behaviour. Although the documentation
> > implied this before, the "optional" property had not been explained
> > properly before.
> > Note that the intended behaviour is the same, this patch just documents
> > it.
> 
> This seems to indicate that it is just a doc change
> 
> > The actual behaviour will change as now absent entries are no longer
> > packed into an image. The image map will also reflect this.
> 
> But this indicates that it changes the functionality. Can you clarify
> this in your commit message?

Will clarify. I wrote that the intended! behaviour is the same, but due to a
bug, the actual behaviour changes ...

The bug being that even though the documentation says optional blobs will be
removed, they are not.

> 
> > As a result, the CheckForProblems() function will no longer alert on
> > optional (blob) entries. This is because the missing optional images
> > were removed before CheckForProblems is called.
> > This, as well, is intended behaviour.
> 
> This breaks testExtblobOptional() for me.

... That is why this test fails I believe. Since it relies on the bug to be
present (i.e. absent optional blobs still included in the image).

> 
> I applied your patch: tools: binman: drop "faked" return value from
> check_fake_fname
> 
> Then I applied this one and got the error.
> 
> I could not apply patch 2 (ools: binman: ftest: pass allow_fake_blob
> to _DoReadFileDtb) as it broke a lot of tests.
> 
> For testExtblobOptional() I see that you have fixed it by the end of
> the series, but I get 11/12 test failures . Are you able to make the
> series bisectable, so that 'binman test' works for each commit? It's
> hard for me to comment on the series in this state.

Yes, will do.

> 
> Also there's no need to put an 'RFC' tag on this series. We do want to
> resolve this, one way or another.

I put RFC, because imo it is not to the quality standards expected of patches.
You wanted to review the current state, but I didn't clean up yet, so that is
why I sent as RFC.

Yannic

> 
> > 
> > Reported-by: Simon Glass <sjg at chromium.org>
> > Signed-off-by: Yannic Moog <y.moog at phytec.de>
> > ---
> >  tools/binman/binman.rst    | 7 +++++++
> >  tools/binman/etype/blob.py | 2 ++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> > index
> > 84b1331df5c006f72167016b7f33de73f4a269fa..392e507d449c75eeba4431fe2db01d820b
> > ce8f61 100644
> > --- a/tools/binman/binman.rst
> > +++ b/tools/binman/binman.rst
> > @@ -1143,6 +1143,13 @@ Optional entries
> > 
> >  Some entries need to exist only if certain conditions are met. For example,
> > an
> >  entry may want to appear in the image only if a file has a particular
> > format.
> > +Also, the ``optional`` property may be used to mark entries as optional::
> > +
> > +    tee-os {
> > +        filename = "tee.bin";
> > +        optional;
> > +    };
> > +
> >  Obviously the entry must exist in the image description for it to be
> > processed
> >  at all, so a way needs to be found to have the entry remove itself.
> > 
> > diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
> > index
> > 970fae91cd9b2a31b3f31d2ecdb77e27a89d69fb..acd9ae34074e4b4b9792b107cc80514dfd
> > d88b3b 100644
> > --- a/tools/binman/etype/blob.py
> > +++ b/tools/binman/etype/blob.py
> > @@ -52,6 +52,8 @@ class Entry_blob(Entry):
> >                  fake_size = self.assume_size
> >              self._pathname = self.check_fake_fname(self._filename,
> > fake_size)
> >              self.missing = True
> > +            if self.optional:
> > +                self.mark_absent("missing but optional")
> >              if not self.faked:
> >                  content_size = 0
> >                  if self.assume_size: # Ensure we get test coverage on next
> > line
> > 
> > --
> > 2.43.0
> > 
> 
> Regards,
> Simon