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

Alper Nebi Yasak
2022-03-03T21:17:45Z

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