Re: [PATCH v3] binman: elf: Check for ELF_TOOLS availability in is_valid

[Tom Rini]

On Tue, Jan 20, 2026 at 06:36:33PM +0100, Quentin Schulz wrote:
> Hi Tom,
> 
> On 1/20/26 4:47 PM, Tom Rini wrote:
> > From: Dmitrii Sharshakov <d3dx12...@gmail.com>
> > 
> > Check if elftools package is available before running DecodeElf().
> > 
> > This clarifies the error message and adds the required test for
> > coverage.
> > 
> > Signed-off-by: Dmitrii Sharshakov <d3dx12...@gmail.com>
> > Signed-off-by: Andrew Soknacki <asokna...@gmail.com>
> > Reviewed-by: Tom Rini <tr...@konsulko.com>
> > [trini: Add the test provided by Andrew on IRC, to fix coverage]
> > Signed-off-by: Tom Rini <tr...@konsulko.com>
> > ---
> > Changes in v3:
> > - Add the test, provided by Andrew, to address the coverage failure in
> >    CI
> > ---
> >   tools/binman/elf.py      |  2 ++
> >   tools/binman/elf_test.py | 12 ++++++++++++
> >   2 files changed, 14 insertions(+)
> > 
> > diff --git a/tools/binman/elf.py b/tools/binman/elf.py
> > index 6ac960e04196..899c84ad36d6 100644
> > --- a/tools/binman/elf.py
> > +++ b/tools/binman/elf.py
> > @@ -570,6 +570,8 @@ def is_valid(data):
> >       Returns:
> >           bool: True if a valid Elf file, False if not
> >       """
> > +    if not ELF_TOOLS:
> > +        raise ValueError("Python: No module named 'elftools'")
> >       try:
> >           DecodeElf(data, 0)
> >           return True
> > diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
> > index 5b1733928986..3ad0bf4c4b09 100644
> > --- a/tools/binman/elf_test.py
> > +++ b/tools/binman/elf_test.py
> > @@ -373,6 +373,18 @@ class TestElf(unittest.TestCase):
> >           self.assertEqual(True, elf.is_valid(data))
> >           self.assertEqual(False, elf.is_valid(data[4:]))
> > +    def test_is_valid_fail(self):
> > +        """Test calling is_valid() without elftools"""
> > +        old_val = elf.ELF_TOOLS
> > +        try:
> > +            elf.ELF_TOOLS = False
> > +            with self.assertRaises(ValueError) as e:
> > +                elf.is_valid(b'')
> > +            self.assertIn("Python: No module named 'elftools'",
> > +                      str(e.exception))
> > +        finally:
> > +            elf.ELF_TOOLS = old_val
> > +
> 
> I'm not sure this is a good idea. The issue is that you're modifying the
> variable from a python module. The check may be run by other tests at the
> same time in different threads and I think the tests will unnecessarily be
> skipped or fail?
> 
> I think we may want something like:
> 
> @unittest.mock.patch('binman.elf.ELF_TOOLS', False)
> def test_is_valid_fail(self):
> ?

It's following the same practice as all of the other tests however. I
might do a follow-up to try what you suggest, since learning about how
to fix this problem yesterday I realized that I can fix I think my "no
cover" in commit 66be03b7ee19 ("binman: blob_dtb: improve error message
when SPL is not found") correctly now, as I think it's the same kind of
failure.

Personally, I find the exercise as an example of testing for tests sake.
We didn't (and can't?) have a test that caught the real problem, which
is a lack of elftools giving a hard to understand error message. The
patch from Dmitrii fixes that real problem (re-use the test+raise logic
other functions do), but python testing requests a test for this new
failure.

-- 
Tom

signature.asc
Description: PGP signature