On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote:
> Also see another attempt for this [1] and the comments to that for a
> more complete picture, though I'll try writing all the points here anyway.
>
> [1] binman: support mkimage separate files
> https://lore.kernel.org/u-boot/20220304195641.1993688-1-pgwipe...@gmail.com/
>
> > ---
> > This is a bit of a messy implementation for now and would probably break
> > existing uses of mkimage that rely on the concatenation behaviour.
>
> I did a `git grep -C10 mkimage -- **/dts/*` and it doesn't look like
> anything uses it yet. Except for binman/test/156_mkimage.dts, which
> doesn't exactly test the concatenation.
I did similar and came to the same conclusion, but I figured I'd ask
just in case it's used somewhere I hadn't found, or it's being relied
on outside of the U-Boot repository.
> You can add a 'separate-files' device-tree property like in [1]. I'm
> actually OK with this separate-files being the default and only behavior
> (concatenation can be done via an inner section), but I'm not sure Simon
> would agree.
Similar thoughts here, especially since we couldn't find anything
relying on the concatenation behaviour.
> > - What kind of test(s) should I add?
>
> At the minimum, a test using separate-files with multiple input entries.
> You can do something like the _HandleGbbCommand in ftest.py to capture
> and check the arguments that'll be passed to mkimage.
>
> I think that'll be enough, but try to run `binman test -T` and check for
> 100% coverage with all tests succeeding.
>
> > (no changes since v1)
> >
> > tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------
> > 1 file changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> > index 5f6def2287..8cea618fbd 100644
> > --- a/tools/binman/etype/mkimage.py
> > +++ b/tools/binman/etype/mkimage.py
> > @@ -51,21 +51,30 @@ class Entry_mkimage(Entry):
>
> Expand the docstring with an explanation of the new behavior, and run
> `binman entry-docs >tools/binman/entries.rst` to update it there as well.
>
> > self.ReadEntries()
> >
> > def ObtainContents(self):
> > - # Use a non-zero size for any fake files to keep mkimage happy
> > - data, input_fname, uniq = self.collect_contents_to_file(
> > - self._mkimage_entries.values(), 'mkimage', 1024)
> > - if data is None:
> > - return False
> > - output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
> > - if self.mkimage.run_cmd('-d', input_fname, *self._args,
> > - output_fname) is not None:
> > + # For multiple inputs to mkimage, we want to separate them by
> > colons.
> > + # This is needed for eg. the rkspi format, which treats the first
> > data
> > + # file as the "init" and the second as "boot" and sets the image
> > header
> > + # accordingly, then makes the image so that only the first 2 KiB
> > of each
> > + # 4KiB block is used.
> > +
> > + data_filenames = []
> > + for entry in self._mkimage_entries.values():
> > + # First get the input data and put it in a file. If any entry
> > is not
> > + # available, try later.
> > + if not entry.ObtainContents():
> > + return False
> > +
> > + input_fname = tools.get_output_filename('mkimage-in.%s' %
> > entry.GetUniqueName())
> > + data_filenames.append(input_fname)
> > + tools.write_file(input_fname, entry.GetData())
>
> Something like collect_contents_to_file([entry], f'mkimage-in-{idx}',
> 1024) would be better here. At least, the files must not be empty (or
> mkimage exits with an error), where entry.GetData() can be b''.
>
> > +
> > + output_fname = tools.get_output_filename('mkimage-out.%s' %
> > self.GetUniqueName())
>
> Should be an f-string.
>
> > + if self.mkimage.run_cmd('-d', ":".join(data_filenames),
> > *self._args, output_fname):
> > self.SetContents(tools.read_file(output_fname))
> > + return True
> > else:
> > - # Bintool is missing; just use the input data as the output
> > self.record_missing_bintool(self.mkimage)
> > - self.SetContents(data)
> > -
> > - return True
> > + return False
>
> This case must set some dummy contents (I'd guess b'' is fine) and
> return True. (False here roughly means "try again later".)
>
> >
> > def ReadEntries(self):
> > """Read the subnodes to find out what should go in this image"""
Thanks for your review! I'll make these updates in the next version.
