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

[Quentin Schulz <quentin.schulz@cherry.de>]

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@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@gmail.com>
>>> Signed-off-by: Andrew Soknacki <asoknacki@gmail.com>
>>> Reviewed-by: Tom Rini <trini@konsulko.com>
>>> [trini: Add the test provided by Andrew on IRC, to fix coverage]
>>> Signed-off-by: Tom Rini <trini@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.

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

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

Quentin