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

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

[Dmitrii Sharshakov]

Check if elftools package is available before running DecodeElf().

This clarifies the error message.

Signed-off-by: Dmitrii Sharshakov <d3dx12.xx@gmail.com>
---
Make the error more informative and indicative of environment issue and
not a code one
---
Changes in v2:
- Reword the commit message as suggested by reviewers
- Link to v1: https://lore.kernel.org/r/20260109-pyelftools-warning-v1-1-1a4965609b55@gmail.com
---
 tools/binman/elf.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/binman/elf.py b/tools/binman/elf.py
index 6ac960e0419..899c84ad36d 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

---
base-commit: 1bcb2fe324180d0a8cfbdb0511737eba1d5b8550
change-id: 20260109-pyelftools-warning-6df5f57ca5b8

Best regards,
-- 
Dmitrii Sharshakov <d3dx12.xx@gmail.com>

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

On Fri, Jan 09, 2026 at 11:03:28PM +0100, Dmitrii Sharshakov wrote:

> Check if elftools package is available before running DecodeElf().
> 
> This clarifies the error message.
> 
> Signed-off-by: Dmitrii Sharshakov <d3dx12.xx@gmail.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Tom Rini]

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
+
     def test_get_symbol_offset(self):
         fname = self.ElfTestFile('embed_data')
         syms = elf.GetSymbols(fname, ['embed_start', 'embed'])
-- 
2.43.0

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

[Quentin Schulz]

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

Cheers,
Quentin

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3379 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Quentin Schulz]

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

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5628 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]