diff mbox series

binman: Correct the error message for a bad hash algorithm

Message ID 20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid
State Accepted
Commit 8db1f9958ff7dfc54ef673607d47694dd672dae7
Delegated to: Simon Glass
Headers show
Series binman: Correct the error message for a bad hash algorithm | expand

Commit Message

Simon Glass Feb. 8, 2022, 5:59 p.m. UTC
This shows an internal type at present, rather than the algorithm name.
Fix it and update the test to catch this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/ftest.py | 2 +-
 tools/binman/state.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Alper Nebi Yasak Feb. 8, 2022, 6:54 p.m. UTC | #1
On 08/02/2022 20:59, Simon Glass wrote:
> This shows an internal type at present, rather than the algorithm name.
> Fix it and update the test to catch this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/binman/ftest.py | 2 +-
>  tools/binman/state.py | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

I saw the failing build for my series [1]. Looks to me like binman
doesn't support crc32 in hash nodes, and turning FIT into a Section
simply exposed that. I tried a sloppy fix, see below.

[1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/388771

> diff --git a/tools/binman/state.py b/tools/binman/state.py
> index af0a65e841..843aefd28d 100644
> --- a/tools/binman/state.py
> +++ b/tools/binman/state.py
> @@ -397,7 +397,7 @@ def CheckAddHashProp(node):
>          if algo.value == 'sha256':
>              size = 32

If I add here:

  +        elif algo.value == 'crc32':
  +            size = 8

and a crc32 implementation attempt to the next function:

>          else:
> -            return "Unknown hash algorithm '%s'" % algo
> +            return "Unknown hash algorithm '%s'" % algo.value
>          for n in GetUpdateNodes(hash_node):
>              n.AddEmptyProp('value', size)

   def CheckSetHashValue(node, get_data_func):
       hash_node = node.FindNode('hash')
       if hash_node:
           algo = hash_node.props.get('algo').value
           if algo == 'sha256':
               m = hashlib.sha256()
               m.update(get_data_func())
               data = m.digest()
  +        elif algo == 'crc32':
  +            data = zlib.crc32(get_data_func()).to_bytes(8, 'little')
           for n in GetUpdateNodes(hash_node):
               n.SetData('value', data)

the failing boards start building again. Not sure how correct this is
though (especially the endianness). Do we need this and maybe a test
with crc32 hash now?
Simon Glass Feb. 8, 2022, 6:57 p.m. UTC | #2
Hi Alper,

On Tue, 8 Feb 2022 at 11:54, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 20:59, Simon Glass wrote:
> > This shows an internal type at present, rather than the algorithm name.
> > Fix it and update the test to catch this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/binman/ftest.py | 2 +-
> >  tools/binman/state.py | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
>
> Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>
> I saw the failing build for my series [1]. Looks to me like binman
> doesn't support crc32 in hash nodes, and turning FIT into a Section
> simply exposed that. I tried a sloppy fix, see below.
>
> [1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/388771
>
> > diff --git a/tools/binman/state.py b/tools/binman/state.py
> > index af0a65e841..843aefd28d 100644
> > --- a/tools/binman/state.py
> > +++ b/tools/binman/state.py
> > @@ -397,7 +397,7 @@ def CheckAddHashProp(node):
> >          if algo.value == 'sha256':
> >              size = 32
>
> If I add here:
>
>   +        elif algo.value == 'crc32':
>   +            size = 8
>
> and a crc32 implementation attempt to the next function:
>
> >          else:
> > -            return "Unknown hash algorithm '%s'" % algo
> > +            return "Unknown hash algorithm '%s'" % algo.value
> >          for n in GetUpdateNodes(hash_node):
> >              n.AddEmptyProp('value', size)
>
>    def CheckSetHashValue(node, get_data_func):
>        hash_node = node.FindNode('hash')
>        if hash_node:
>            algo = hash_node.props.get('algo').value
>            if algo == 'sha256':
>                m = hashlib.sha256()
>                m.update(get_data_func())
>                data = m.digest()
>   +        elif algo == 'crc32':
>   +            data = zlib.crc32(get_data_func()).to_bytes(8, 'little')
>            for n in GetUpdateNodes(hash_node):
>                n.SetData('value', data)
>
> the failing boards start building again. Not sure how correct this is
> though (especially the endianness). Do we need this and maybe a test
> with crc32 hash now?

I can get your series in without the final patch (testing at
u-boot-dm/testing now).

The thing is that mkimage creates the hashes so we don't want binman
doing that too.

My current feeling is that we need a way to tell sections not to add
calculated properties for hashes...but just for the FIT section
itself, so its children should still add these.

Regards,
Simon
Simon Glass Feb. 23, 2022, 2:35 a.m. UTC | #3
Hi Alper,

On Tue, 8 Feb 2022 at 11:54, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 20:59, Simon Glass wrote:
> > This shows an internal type at present, rather than the algorithm name.
> > Fix it and update the test to catch this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/binman/ftest.py | 2 +-
> >  tools/binman/state.py | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
>
> Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>
> I saw the failing build for my series [1]. Looks to me like binman
> doesn't support crc32 in hash nodes, and turning FIT into a Section
> simply exposed that. I tried a sloppy fix, see below.
>
> [1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/388771
>
Applied to u-boot-dm, thanks!
diff mbox series

Patch

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 5400f76c67..3e15de63d5 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -2073,7 +2073,7 @@  class TestFunctional(unittest.TestCase):
     def testHashBadAlgo(self):
         with self.assertRaises(ValueError) as e:
             self._DoReadFileDtb('092_hash_bad_algo.dts', update_dtb=True)
-        self.assertIn("Node '/binman/u-boot': Unknown hash algorithm",
+        self.assertIn("Node '/binman/u-boot': Unknown hash algorithm 'invalid'",
                       str(e.exception))
 
     def testHashSection(self):
diff --git a/tools/binman/state.py b/tools/binman/state.py
index af0a65e841..843aefd28d 100644
--- a/tools/binman/state.py
+++ b/tools/binman/state.py
@@ -397,7 +397,7 @@  def CheckAddHashProp(node):
         if algo.value == 'sha256':
             size = 32
         else:
-            return "Unknown hash algorithm '%s'" % algo
+            return "Unknown hash algorithm '%s'" % algo.value
         for n in GetUpdateNodes(hash_node):
             n.AddEmptyProp('value', size)