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

[Simon Glass]

Hi Quentin,

On Mon, 7 Nov 2022 at 07:17, Quentin Schulz
<quentin.schulz at theobroma-systems.com> wrote:
>
> 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?]

will update

>
> > +    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?

I am not sure how to do that, but have updated help.

>
> >       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?

I'll rename the option as the intent is to use it for missing blobs
and bintools, since they both result in the same thing. A faked blob
is just a subset of a missing blob, so there really just needs to be
an --allow-missing option.

>
> 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?

No, will update this code.

Regards,
Simon