| Message ID | 20220223230040.159317-13-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script | expand |
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 > > [...]
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
On 06/03/2022 06:08, Simon Glass wrote: > On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: >> On 24/02/2022 02:00, Simon Glass wrote: >>> +- 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. But the mkimage etype creates its own files to pass to mkimage anyway, using entry.GetData() in collect_contents_to_file() without reaching into subentries' files. More specifically, what I'm thinking of is removing check_fake_fname() entirely, instead of managing/removing the files it creates. At a cursory glance, only blob and blob-ext-list seem to use that directly.
Hi Alper, On Thu, 10 Mar 2022 at 12:36, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > > On 06/03/2022 06:08, Simon Glass wrote: > > On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > >> On 24/02/2022 02:00, Simon Glass wrote: > >>> +- 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. > > But the mkimage etype creates its own files to pass to mkimage anyway, > using entry.GetData() in collect_contents_to_file() without reaching > into subentries' files. > > More specifically, what I'm thinking of is removing check_fake_fname() > entirely, instead of managing/removing the files it creates. At a > cursory glance, only blob and blob-ext-list seem to use that directly. OK, but if the files don't get created, then mkimage will fail, right? Regards, Simon
On 12/03/2022 08:02, Simon Glass wrote: > On Thu, 10 Mar 2022 at 12:36, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: >> On 06/03/2022 06:08, Simon Glass wrote: >>> On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: >>>> 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. >> >> But the mkimage etype creates its own files to pass to mkimage anyway, >> using entry.GetData() in collect_contents_to_file() without reaching >> into subentries' files. >> >> More specifically, what I'm thinking of is removing check_fake_fname() >> entirely, instead of managing/removing the files it creates. At a >> cursory glance, only blob and blob-ext-list seem to use that directly. > > OK, but if the files don't get created, then mkimage will fail, right? I don't think it will, but maybe it's better that I try removing it to see if anything breaks.
Hi Alper, On Mon, 14 Mar 2022 at 15:31, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > > On 12/03/2022 08:02, Simon Glass wrote: > > On Thu, 10 Mar 2022 at 12:36, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > >> On 06/03/2022 06:08, Simon Glass wrote: > >>> On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > >>>> 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. > >> > >> But the mkimage etype creates its own files to pass to mkimage anyway, > >> using entry.GetData() in collect_contents_to_file() without reaching > >> into subentries' files. > >> > >> More specifically, what I'm thinking of is removing check_fake_fname() > >> entirely, instead of managing/removing the files it creates. At a > >> cursory glance, only blob and blob-ext-list seem to use that directly. > > > > OK, but if the files don't get created, then mkimage will fail, right? > > I don't think it will, but maybe it's better that I try removing it to > see if anything breaks. Yes, the reason for adding faked blobs was needing to run mkimage which expects files (in the .its or elsewhere) to be present. Regards, Simon
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 -- 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 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 diff --git a/tools/binman/etype/blob_ext_list.py b/tools/binman/etype/blob_ext_list.py index 76ad32a1ee..f00202e9eb 100644 --- a/tools/binman/etype/blob_ext_list.py +++ b/tools/binman/etype/blob_ext_list.py @@ -37,7 +37,7 @@ class Entry_blob_ext_list(Entry_blob): missing = False pathnames = [] for fname in self._filenames: - fname = self.check_fake_fname(fname) + fname, _ = self.check_fake_fname(fname) pathname = tools.get_input_filename( fname, self.external and self.section.GetAllowMissing()) # Allow the file to be missing
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(-)