Re: [PATCH v2 12/25] binman: Change how faked blobs are created

[Simon Glass <sjg@chromium.org>]

Hi Alper,

On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 24/02/2022 02:00, Simon Glass wrote:
> > At present fake blobs are created but internally an empty blob is used.
> > Change it to use the contents of the faked file. Also return whether the
> > blob was faked, in case the caller needs to know that.
> >
> > Add a TODO to put fake blobs in their own directory.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Add a patch to change how faked blobs are created
> >
> >  tools/binman/binman.rst             | 3 ++-
> >  tools/binman/entry.py               | 9 ++++++---
> >  tools/binman/etype/blob.py          | 7 ++++---
> >  tools/binman/etype/blob_ext_list.py | 2 +-
> >  4 files changed, 13 insertions(+), 8 deletions(-)
>
> I feel a bit uneasy about all this fake file stuff, but can't figure out
> exactly why. I guess the patch doesn't make it that worse.

Well we have at least one major problem with it, which is that fake
blobs need to exist only for the life of the binman run.

>
> Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>
> > diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> > index 85f8337a31..a90364f2fb 100644
> > --- a/tools/binman/binman.rst
> > +++ b/tools/binman/binman.rst
> > @@ -1500,7 +1500,8 @@ Some ideas:
> >  - Figure out how to make Fdt support changing the node order, so that
> >    Node.AddSubnode() can support adding a node before another, existing node.
> >    Perhaps it should completely regenerate the flat tree?
> > -
> > +- Put faked files into a separate subdir and remove them on start-up, to avoid
> > +  seeing them as 'real' files on a subsequent run
>
> Do we need to create actual files, or is it a convenience thing for blob
> entry types (because they already read their contents from files)?

We need the files for mkimage.

>
> Also, maybe it's better to create them somewhere in the /tmp/binman.*
> directories so they disappear without effort.

Yes that would be better. I decided to stop this series where it is
since that can be done in a follow-up. I did add a TODO though.

>
> >
> >  --
> >  Simon Glass <sjg@chromium.org>
> > diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> > index 00a13c5839..2fb0050da5 100644
> > --- a/tools/binman/entry.py
> > +++ b/tools/binman/entry.py
> > @@ -997,15 +997,18 @@ features to produce new behaviours.
> >              fname (str): Filename to check
> >
> >          Returns:
> > -            fname (str): Filename of faked file
> > +            tuple:
> > +                fname (str): Filename of faked file
> > +                bool: True if the blob was faked, False if not
> >          """
> >          if self.allow_fake and not pathlib.Path(fname).is_file():
> >              outfname = tools.get_output_filename(os.path.basename(fname))
> >              with open(outfname, "wb") as out:
> >                  out.truncate(1024)
> >              self.faked = True
> > -            return outfname
> > -        return fname
> > +            tout.info(f"Entry '{self._node.path}': Faked file '{outfname}'")
> > +            return outfname, True
> > +        return fname, False
>
> Can't callers use self.faked for this?
>
> I think I see an edge case when calling this multiple times for the same
> filename, only the first call would recognize it being a fake file and
> only the first-entry-to-call would consider itself faked.

Yes we could and yes there is an edge case. This function returns True
if it created the fake, False if it had happened before. I decided to
leave it as is since checking self.faked after a function that sets it
seems like a side effect. No strong opinions on it though.

>
> >
> >      def CheckFakedBlobs(self, faked_blobs_list):
> >          """Check if any entries in this section have faked external blobs
> > diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
> > index 25ec5d26c9..89f089e740 100644
> > --- a/tools/binman/etype/blob.py
> > +++ b/tools/binman/etype/blob.py
> > @@ -41,10 +41,11 @@ class Entry_blob(Entry):
> >              self.external and self.section.GetAllowMissing())
> >          # Allow the file to be missing
> >          if not self._pathname:
> > -            self._pathname = self.check_fake_fname(self._filename)
> > -            self.SetContents(b'')
> > +            self._pathname, faked = self.check_fake_fname(self._filename)
> >              self.missing = True
> > -            return True
> > +            if not faked:
> > +                self.SetContents(b'')
> > +                return True
> >
> >          self.ReadBlobContents()
> >          return True
> >
> > [...]

Regards,
Simon