[RFC PATCH v2 1/8] binman: mkimage: Support ':'-separated inputs

[Andrew Abbott]

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