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

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

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.

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)?

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

>  
>  --
>  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.

>  
>      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
>  
> [...]