[PATCH v3 3/9] binman: Use an exit code when blobs are missing

[Quentin Schulz]

Hi Simon,

On 11/6/22 00:04, Simon Glass wrote:
> At present binman returns success when told to handle missing blobs.
> This is confusing this in fact the resulting image cannot work.
> 
> Use exit code 103 to signal this problem, with a -W option to convert
> it to a warning.
> 
> Add documentation about exit codes while we are here.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
> (no changes since v1)
> 
>   tools/binman/binman.rst | 21 +++++++++++++++++++++
>   tools/binman/cmdline.py |  3 +++
>   tools/binman/control.py |  7 ++++++-
>   tools/binman/ftest.py   | 20 ++++++++++++++++++--
>   4 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> index fda16f1992d..30ced31e43b 100644
> --- a/tools/binman/binman.rst
> +++ b/tools/binman/binman.rst
> @@ -1461,6 +1461,10 @@ space-separated list of directories to search for binary blobs::
>          odroid-c4/build/board/hardkernel/odroidc4/firmware \
>          odroid-c4/build/scp_task" binman ...
>   
> +Note that binman fails with exit code 103 when there are missing blobs. If you
> +wish binman to continue anyway, you can pass `-W` to binman.
> +
> +
>   Code coverage
>   -------------
>   
> @@ -1472,6 +1476,23 @@ To enable Python test coverage on Debian-type distributions (e.g. Ubuntu)::
>      $ sudo apt-get install python-coverage python3-coverage python-pytest
>   
>   
> +Exit status
> +-----------
> +
> +Binman produces the following exit codes:
> +
> +0
> +    Success
> +
> +1
> +    Any sort of failure - see output for more details
> +
> +103
> +    There are missing external blobs or bintools. This is only returned if
> +    -M is passed to binman, otherwise missing blobs return in an exit status of

and if -W is *not* passed to binman, no?

> +    1.
> +
> +
>   Error messages
>   --------------
>   
> diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py
> index 1d1ca43993d..144c0c916a2 100644
> --- a/tools/binman/cmdline.py
> +++ b/tools/binman/cmdline.py
> @@ -128,6 +128,9 @@ controlled by a description in the board device tree.'''
>           default=False, help='Update the binman node with offset/size info')
>       build_parser.add_argument('--update-fdt-in-elf', type=str,
>           help='Update an ELF file with the output dtb: infile,outfile,begin_sym,end_sym')
> +    build_parser.add_argument(
> +        '-W', '--ignore-missing-blobs', action='store_true', default=False,
> +        help='Return success even if there are missing blobs')
>   

Is -W supposed to be able to be used without --allow-missing? If not, I 
think it could make sense to use a subparser here to have the -W option 
available only when -M is selected?

>       subparsers.add_parser(
>           'bintool-docs', help='Write out bintool documentation (see bintool.rst)')
> diff --git a/tools/binman/control.py b/tools/binman/control.py
> index bfe63a15204..8b94db60113 100644
> --- a/tools/binman/control.py
> +++ b/tools/binman/control.py
> @@ -742,7 +742,12 @@ def Binman(args):
>                   elf.UpdateFile(*elf_params, data)
>   
>               if invalid:
> -                tout.warning("\nSome images are invalid")
> +                msg = '\nSome images are invalid'
> +                if args.ignore_missing_blobs:
> +                    tout.warning(msg)
> +                else:
> +                    tout.error(msg)
> +                    return 103

I think this could be an issue.

invalid can mean either missing blob, faked blob or missing btool. Here, 
we only want to warn/fail in case of missing blob and not the other two?

Also, shouldn't we have this only if we have allow_missing set?

>   
>               # Use this to debug the time take to pack the image
>               #state.TimingShow()
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index e849d96587c..fc38a2efccd 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -340,7 +340,7 @@ class TestFunctional(unittest.TestCase):
>                       use_expanded=False, verbosity=None, allow_missing=False,
>                       allow_fake_blobs=False, extra_indirs=None, threads=None,
>                       test_section_timeout=False, update_fdt_in_elf=None,
> -                    force_missing_bintools=''):
> +                    force_missing_bintools='', ignore_missing_blobs=False):
>           """Run binman with a given test file
>   
>           Args:
> @@ -403,6 +403,8 @@ class TestFunctional(unittest.TestCase):
>                   args.append('-a%s=%s' % (arg, value))
>           if allow_missing:
>               args.append('-M')
> +        if ignore_missing_blobs:
> +            args.append('-W')

Can -W really be used when -M is not passed?

Cheers,
Quentin