[PATCH v2 6/6] RFC: binman: Improve allow missing for mkimage entry

[Simon Glass]

Hi Jonas,

On Tue, 14 Feb 2023 at 03:34, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> Implement CheckMissing and CheckOptional methods that is adapted to
> Entry_mkimage in order to improve support for allow missing flag.
>
> Use collect_contents_to_file in multiple-data-files handling to improve
> support for allow missing and fake blobs handling.
>
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
> ---
> Building for RK3568 without ROCKCHIP_TPL will result in the following
> error message, regardless if BINMAN_ALLOW_MISSING is used or not.
>
>   binman: Filename 'rockchip-tpl' not found in input path (...)
>
> With this patch and using BINMAN_ALLOW_MISSING=1 a new external blob
> with no content is created and then passed to mkimage, resulting in an
> error from the mkimage command.
>
>   binman: Error 255 running 'mkimage -d ./mkimage-0.simple-bin.mkimage:./mkimage-1.simple-bin.mkimage -n rk3568 -T rksd ./idbloader.img': mkimage: Can't read ./mkimage-0.simple-bin.mkimage: Invalid argument
>
> If the --fake-ext-blobs argument is also used I get the message I was
> expecting to see from the beginning.
>
>   Image 'main-section' is missing external blobs and is non-functional: rockchip-tpl
>
>   /binman/simple-bin/mkimage/rockchip-tpl:
>      An external TPL is required to initialize DRAM. Get the external TPL
>      binary and build with ROCKCHIP_TPL=/path/to/ddr.bin. One possible source
>      for the external TPL binary is https://github.com/rockchip-linux/rkbin.
>   Image 'main-section' has faked external blobs and is non-functional: rockchip-tpl
>
>   Some images are invalid
>
> Not sure how this should work, but I was expecting to see the message
> about the missing rockchip-tpl from the beginning instead of the generic
> "not found in input path" message. At leas with BINMAN_ALLOW_MISSING=1.
>
>  tools/binman/etype/mkimage.py | 37 +++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index cb264c3cad0b..44510a8c40ba 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -153,10 +153,12 @@ class Entry_mkimage(Entry):
>          if self._multiple_data_files:
>              fnames = []
>              uniq = self.GetUniqueName()
> -            for entry in self._mkimage_entries.values():
> -                if not entry.ObtainContents(fake_size=fake_size):
> +            for idx, entry in enumerate(self._mkimage_entries.values()):
> +                entry_data, entry_fname, _ = self.collect_contents_to_file(
> +                    [entry], 'mkimage-%s' % idx, fake_size)
> +                if entry_data is None:

This is OK, I suppose, but I'm not quite sure what actually changes
here, other than writing each entry to a file?

Also, if you do this, please add / extend a test that checks that the
output files are written, since there is otherwise no coverage here.

What test uses these optional mkimage pieces?

>                      return False
> -                fnames.append(tools.get_input_filename(entry.GetDefaultFilename()))
> +                fnames.append(entry_fname)
>              input_fname = ":".join(fnames)
>          else:
>              data, input_fname, uniq = self.collect_contents_to_file(
> @@ -165,7 +167,7 @@ class Entry_mkimage(Entry):
>                  return False
>          if self._imagename:
>              image_data, imagename_fname, _ = self.collect_contents_to_file(
> -                [self._imagename], 'mkimage-n', 1024)
> +                [self._imagename], 'mkimage-n', fake_size)
>              if image_data is None:
>                  return False
>          outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq
> @@ -216,6 +218,20 @@ class Entry_mkimage(Entry):
>          if self._imagename:
>              self._imagename.SetAllowFakeBlob(allow_fake)
>
> +    def CheckMissing(self, missing_list):
> +        """Check if any entries in this section have missing external blobs
> +
> +        If there are missing (non-optional) blobs, the entries are added to the
> +        list
> +
> +        Args:
> +            missing_list: List of Entry objects to be added to
> +        """
> +        for entry in self._mkimage_entries.values():
> +            entry.CheckMissing(missing_list)
> +        if self._imagename:
> +            self._imagename.CheckMissing(missing_list)
> +
>      def CheckFakedBlobs(self, faked_blobs_list):
>          """Check if any entries in this section have faked external blobs
>
> @@ -229,6 +245,19 @@ class Entry_mkimage(Entry):
>          if self._imagename:
>              self._imagename.CheckFakedBlobs(faked_blobs_list)
>
> +    def CheckOptional(self, optional_list):
> +        """Check the section for missing but optional external blobs
> +
> +        If there are missing (optional) blobs, the entries are added to the list
> +
> +        Args:
> +            optional_list (list): List of Entry objects to be added to
> +        """
> +        for entry in self._mkimage_entries.values():
> +            entry.CheckOptional(optional_list)
> +        if self._imagename:
> +            self._imagename.CheckOptional(optional_list)
> +
>      def AddBintools(self, btools):
>          super().AddBintools(btools)
>          self.mkimage = self.AddBintool(btools, 'mkimage')
> --
> 2.39.1
>

Regards,
Simon