[PATCH] binman: Support optional external blobs

[Quentin Schulz]

Hi Simon,

On 12/17/22 22:29, Simon Glass wrote:
> Some blobs are actually not necessary for the board to work correctly. Add
> a property to allow this to be indicated. Missing optional blobs do not
> cause a build failure.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
>   tools/binman/binman.rst                |  9 +++++++++
>   tools/binman/control.py                | 11 +++++++++++
>   tools/binman/entry.py                  | 25 +++++++++++++++++++++----
>   tools/binman/etype/blob.py             |  2 +-
>   tools/binman/etype/section.py          | 14 +++++++++++++-
>   tools/binman/ftest.py                  | 10 ++++++++++
>   tools/binman/test/266_blob_ext_opt.dts | 21 +++++++++++++++++++++
>   7 files changed, 86 insertions(+), 6 deletions(-)
>   create mode 100644 tools/binman/test/266_blob_ext_opt.dts
> 
> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> index 1ab6d012b83..391494907ab 100644
> --- a/tools/binman/binman.rst
> +++ b/tools/binman/binman.rst
> @@ -689,6 +689,15 @@ no-expanded:
>       `no-expanded` property disables this just for a single entry. Put the
>       `no-expanded` boolean property in the node to select this behaviour.
>   
> +optional:
> +    External blobs are normally required to be present for the image to be
> +    built (but see `External blobs`_). This properly allows an entry to be
> +    optional, so that when it is cannot be found, this problem is ignored and
> +    an empty file is used for this blob. This should be used only when the blob
> +    is entirely optional and is not needed for correct operation of the image.

Could we provide such an example?

Are you thinking about OP-TEE for example?

I'm also a bit worried because warnings are notoriously ignored by users 
if builds do not end up failing. At least binman usually runs at the end 
of the build, so the warnings wouldn't be hidden in a pile of compiler 
warnings (thinking about GCC upgrades or vendor u-boot).

> +    Note that missing, optional blobs do not produce a non-zero exit code from
> +    binman, although it does show a warning about the missing external blob.
> + >   The attributes supported for images and sections are described 
below. Several
>   are similar to those for entries.
>   
> diff --git a/tools/binman/control.py b/tools/binman/control.py
> index 07225381146..e64740094f6 100644
> --- a/tools/binman/control.py
> +++ b/tools/binman/control.py
> @@ -594,12 +594,14 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
>       image.BuildImage()
>       if write_map:
>           image.WriteMap()
> +
>       missing_list = []
>       image.CheckMissing(missing_list)
>       if missing_list:
>           tout.warning("Image '%s' is missing external blobs and is non-functional: %s" %
>                        (image.name, ' '.join([e.name for e in missing_list])))
>           _ShowHelpForMissingBlobs(missing_list)
> +
>       faked_list = []
>       image.CheckFakedBlobs(faked_list)
>       if faked_list:
> @@ -607,6 +609,15 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
>               "Image '%s' has faked external blobs and is non-functional: %s" %
>               (image.name, ' '.join([os.path.basename(e.GetDefaultFilename())
>                                      for e in faked_list])))
> +
> +    optional_list = []
> +    image.CheckOptional(optional_list)
> +    if optional_list:
> +        tout.warning(
> +            "Image '%s' is missing external blobs but is still functional: %s" %
> +            (image.name, ' '.join([e.name for e in optional_list])))
> +        _ShowHelpForMissingBlobs(optional_list)
> +
>       missing_bintool_list = []
>       image.check_missing_bintools(missing_bintool_list)
>       if missing_bintool_list:
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index de51d295891..d73f3013405 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -73,7 +73,9 @@ class Entry(object):
>           compress: Compression algoithm used (e.g. 'lz4'), 'none' if none
>           orig_offset: Original offset value read from node
>           orig_size: Original size value read from node
> -        missing: True if this entry is missing its contents
> +        missing: True if this entry is missing its contents. Note that if it is
> +            optional, this entry will not appear in the list generated by
> +            entry.CheckMissing() since it is considered OK for it to be missing.
>           allow_missing: Allow children of this entry to be missing (used by
>               subclasses such as Entry_section)
>           allow_fake: Allow creating a dummy fake file if the blob file is not
> @@ -95,6 +97,7 @@ class Entry(object):
>               the entry itself, allowing it to vanish in certain circumstances.
>               An absent entry is removed during processing so that it does not
>               appear in the map
> +        optional (bool): True if this entry contains an optional external blob
>       """
>       fake_dir = None
>   
> @@ -138,6 +141,7 @@ class Entry(object):
>           self.elf_fname = None
>           self.auto_write_symbols = auto_write_symbols
>           self.absent = False
> +        self.optional = False
>   
>       @staticmethod
>       def FindEntryClass(etype, expanded):
> @@ -289,6 +293,7 @@ class Entry(object):
>           self.offset_unset = fdt_util.GetBool(self._node, 'offset-unset')
>           self.extend_size = fdt_util.GetBool(self._node, 'extend-size')
>           self.missing_msg = fdt_util.GetString(self._node, 'missing-msg')
> +        self.optional = fdt_util.GetBool(self._node, 'optional')
>   
>           # This is only supported by blobs and sections at present
>           self.compress = fdt_util.GetString(self._node, 'compress', 'none')
> @@ -1039,14 +1044,15 @@ features to produce new behaviours.
>           self.allow_fake = allow_fake
>   
>       def CheckMissing(self, missing_list):
> -        """Check if any entries in this section have missing external blobs
> +        """Check if the entry has missing external blobs
>   
> -        If there are missing blobs, the entries are added to the list
> +        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
>           """
> -        if self.missing:
> +        if self.missing and not self.optional:
>               missing_list.append(self)
>   

I'm tempted to have CheckMissing operate on two lists, one optional and 
one required? I'm not sure there's a need to do separate calls for 
optional and required missing blobs and it would make it harder to not 
forget about checking those in the right places.

>       def check_fake_fname(self, fname, size=0):
> @@ -1085,6 +1091,17 @@ features to produce new behaviours.
>           # This is meaningless for anything other than blobs
>           pass
>   
> +    def CheckOptional(self, optional_list):

If still going for separate functions, I would at least name this one 
CheckOptionalMissing to make it a bit more obvious what we're after 
without reading the function documentation.

Cheers,
Quentin