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

[Tom Rini]

On Tue, Jan 20, 2026 at 07:15:08PM +0100, Quentin Schulz wrote:
> Hi Tom,
> 
> On 1/20/26 6:55 PM, Tom Rini wrote:
> > 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.xx at 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.xx at gmail.com>
> > > > Signed-off-by: Andrew Soknacki <asoknacki at gmail.com>
> > > > Reviewed-by: Tom Rini <trini at konsulko.com>
> > > > [trini: Add the test provided by Andrew on IRC, to fix coverage]
> > > > Signed-off-by: Tom Rini <trini at 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
> 
> Which is probably incorrect. Also not sure about the os.environ
> modification... probably should also be a global or temporary mock.

It's well beyond my scope of expertise at this point, yes.

> > 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.
> > 
> 
> Yeah, you can likely mock self.FdtContents() to return None, None to trigger
> the FileNotFoundError exception with the desired parameters. The only issue
> is whether self.FdtContents is called multiple times within the same
> function (maybe from functions/methods called from that function), then it
> gets harder.
> 
> > Personally, I find the exercise as an example of testing for tests sake.
> 
> Coverage test is difficult. Sometimes it's unnecessary tests so it makes the
> coverage tool happy we don't regress. How does one decide which tests mean
> something /me shrugs.

Yes, I agree it's a hard task to solve, and sometimes you just need to
put in a test to make the tool happy.

> > 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.
> > 
> 
> I haven't looked much into it, but reading elf.py, I didn't like that some
> methods require elftools and some not. I'm wondering if we shouldn't split
> them in two and have the one that relies on elftools not even check if it's
> there. It imports the module, done. If it cannot, it'll complain about
> NameError: name 'ELFFile' is not defined
> NameError: name 'ELFError' is not defined. Did you mean: 'EOFError'?
> and it's up to the caller to handle whether it should be a hard failure (not
> catch it) or not?
> or we could also just raise an Exception to tell the user to install
> pyelftools if we really need a hint at what to do when the exception is
> raised.

That's one of the first things I noticed as well when I saw v1 of this
patch and wanted to improve it. I agree splitting the file and having
one try/raise would be better, and split the requires elftools code from
the doesn't require elftools code.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20260120/3dbbe0d2/attachment.sig>