[PATCH 16/24] binman: Move entry-data collection into a Entry method

[Alper Nebi Yasak]

On 08/02/2022 21:50, Simon Glass wrote:
> Collecting the data from a list of entries and putting it in a file is
> a useful operation that will be needed by other entry types. Put this into
> a method in the Entry class.
> 
> Add some documentation about how to collect data for an entry type.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
>  tools/binman/binman.rst       | 86 +++++++++++++++++++++++++++++++++++
>  tools/binman/entry.py         | 28 ++++++++++++
>  tools/binman/etype/mkimage.py | 14 ++----
>  3 files changed, 118 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> index d6b95de1f5..771645380e 100644
> --- a/tools/binman/binman.rst
> +++ b/tools/binman/binman.rst
> @@ -1358,6 +1358,92 @@ development, since dealing with exceptions and problems in threads is more
>  difficult. This avoids any use of ThreadPoolExecutor.
>  
>  
> +Collecting data for an entry type
> +---------------------------------
> +
> +Some entry types deal with data obtained from others. For example,
> +`Entry_mkimage` calls the `mkimage` tool with data from its subnodes::
> +
> +    mkimage {
> +        args = "-n test -T script";
> +
> +        u-boot-spl {
> +        };
> +
> +        u-boot {
> +        };
> +    };
> +
> +This shows mkimage being passed a file consisting of SPL and U-Boot proper. It
> +is create by calling `Entry.collect_contents_to_file()`. Note that in this case,

create -> created

> +the data is passed to mkimage for processing but does not appear separately in
> +the image. It may not appear at all, depending on what mkimage does. The
> +contents of the `mkimage` entry are entirely dependent on the processing done
> +by the entry, with the provided subnodes (`u-boot-spl` and `u-boot`) simply
> +providing the input data for that processing.
> +
> +Note that `Entry.collect_contents_to_file()` simply concatenates the data from
> +the different entries together, with no control over alignment, etc. Another
> +approach is to subclass `Entry_section` so that those features become available,
> +such as `size` and `pad-byte`. Then the contents of the entry can be obtained by
> +calling `BuildSectionData()`.

Just saying BuildSectionData() sounds a bit unclear. I think you mean
that subclasses can call super().BuildSectionData() in their
BuildSectionData() implementation to get the input data, then write it
to a file and process it however they like into the final entry data.

After subclassing FIT from Section I've been thinking that any entry
that has subnodes for entries could be likewise made into a Section to
reuse its subentry management code. Anything preventing that for mkimage
and others that might use collect_contents_to_file?

> +
> +There are other ways to obtain data also, depending on the situation. If the
> +entry type is simply signing data which exists elsewhere in the image, then
> +you can use `Entry_collection`  as a base class. It lets you use a property
> +called `content` which lists the entries containing data to be processed. This
> +is used by `Entry_vblock`, for example::
> +
> +    u_boot: u-boot {
> +    };

I like putting spaces between sibling nodes, could use one here and in
atf-fip below for consistency.

> +    vblock {
> +        content = <&u_boot &dtb>;
> +        keyblock = "firmware.keyblock";
> +        signprivate = "firmware_data_key.vbprivk";
> +        version = <1>;
> +        kernelkey = "kernel_subkey.vbpubk";
> +        preamble-flags = <1>;
> +    };
> +
> +    dtb: u-boot-dtb {
> +    };
> +
> +which shows an image containing `u-boot` and `u-boot-dtb`, with the `vblock`
> +image collecting their contents to produce input for its signing process,
> +without affecting those entries, which still appear in the final image
> +untouched.
> +
> +Another example is where an entry type needs several independent pieces of input
> +to function. For example, `Entry_fip` allows a number of different binary blobs
> +to be placed in their own individual places in a custom data structure in the
> +output image. To make that work you can add subnodes for each of them and call
> +`Entry.Create()` on each subnode, as `Entry_fip` does. Then the data for each
> +blob can come from any suitable place, such as an `Entry_u_boot` or an
> +`Entry_blob` or anything else::
> +
> +    atf-fip {
> +        fip-hdr-flags = /bits/ 64 <0x123>;
> +        soc-fw {
> +            fip-flags = /bits/ 64 <0x123456789abcdef>;
> +            filename = "bl31.bin";
> +        };
> +
> +        u-boot {
> +            fip-uuid = [fc 65 13 92 4a 5b 11 ec
> +                    94 35 ff 2d 1c fc 79 9c];
> +        };
> +    };
> +
> +The `soc-fw` node is a `blob-ext` (i.e. it reads in a named binary file) whereas
> +`u-boot` is a normal entry type. This works because `Entry_fip` selects the
> +`blob-ext` entry type if the node name (here `soc-fw`) is recognised as being
> +a known blob type.
> +
> +When adding new entry types you are encouraged to use subnodes to provide the
> +data for processing, unless the `content` approach is more suitable. Ad-hoc
> +properties and other methods of obtaining data are discouraged, since it adds to
> +confusion for users.

I think a good way to make this decision is to consider whether the
input entries are contained within (or consumed by) the entry, vs just
being 'referenced' by the entry.

> +
>  History / Credits
>  -----------------
>  
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 08770ec5f0..3eafa078ae 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -1108,3 +1108,31 @@ features to produce new behaviours.
>          btool = bintool.Bintool.create(name)
>          tools[name] = btool
>          return btool
> +
> +    def collect_contents_to_file(self, entries, prefix):
> +        """Put the contents of a list of entries into a file
> +
> +        Args:
> +            entries (list of Entry): Entries to collect
> +            prefix (str): Filename prefix of file to write to
> +
> +        If any entry does not have contents yet, this function returns False
> +        for the data.
> +
> +        Returns:
> +            Tuple:
> +                bytes: Concatenated data from all the entries (or False)
> +                str: Filename of file written (or False if no data)
> +                str: Unique portion of filename (or False if no data)
> +        """
> +        data = b''
> +        for entry in entries:
> +            # First get the input data and put it in a file. If not available,
> +            # try later.
> +            if not entry.ObtainContents():
> +                return False, False, False

I'd prefer None over False for return types other than bool.

> +            data += entry.GetData()
> +        uniq = self.GetUniqueName()
> +        fname = tools.GetOutputFilename(f'{prefix}.{uniq}')
> +        tools.WriteFile(fname, data)
> +        return data, fname, uniq
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 9ecd1c2548..145a50f1aa 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -51,16 +51,10 @@ class Entry_mkimage(Entry):
>          self.ReadEntries()
>  
>      def ObtainContents(self):
> -        data = b''
> -        for entry in self._mkimage_entries.values():
> -            # First get the input data and put it in a file. If not available,
> -            # try later.
> -            if not entry.ObtainContents():
> -                return False
> -            data += entry.GetData()
> -        uniq = self.GetUniqueName()
> -        input_fname = tools.GetOutputFilename('mkimage.%s' % uniq)
> -        tools.WriteFile(input_fname, data)
> +        data, input_fname, uniq = self.collect_contents_to_file(
> +            self._mkimage_entries.values(), 'mkimage')
> +        if data is False:
> +            return False
>          output_fname = tools.GetOutputFilename('mkimage-out.%s' % uniq)
>          if self.mkimage.run_cmd('-d', input_fname, *self._args,
>                                  output_fname) is not None: