[PATCH RFC] binman: Handle some newer pylint warnings

[Simon Glass]

Hi Tom,

On Tue, 18 Feb 2025 at 12:25, Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, Feb 12, 2025 at 04:22:59PM -0600, Tom Rini wrote:
>
> > With a newer pylint we have a few warnings show up now that are harder
> > to handle. The first problem is that when we call self.Raise(...) this
> > is no longer enough to have pylint see that we're now bailing out and
> > won't attempt to reference an unassigned variable. This is all of the
> > pylint disabling, with the exception of _AddNode.
> >
> > The problem with _AddNode is that before we call this function we set
> > fsw as libfdt.FdtSw() and I'm unsure how best to rework the code.
> >
> > The final problem is in CheckSetHashValue() where the function only
> > handles one algorithm but the intention is to handle others as well.
> > Once another algorithm is added and then also error handling for no
> > algorithm is added, we would not attempt to use data before it's been
> > assigned.
> >
> > As I'm not confident all of these are the right way to handle the
> > problem I've marked this as RFC and not split the changes up more.
> >
> > Signed-off-by: Tom Rini <trini at konsulko.com>
> > ---
> > Cc: Simon Glass <sjg at chromium.org>
> > ---
> >  tools/binman/etype/fdtmap.py       | 1 +
> >  tools/binman/etype/image_header.py | 1 +
> >  tools/binman/etype/pre_load.py     | 1 +
> >  tools/binman/etype/x509_cert.py    | 1 +
> >  tools/binman/ftest.py              | 1 +
> >  tools/binman/state.py              | 4 ++--
> >  6 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/binman/etype/fdtmap.py b/tools/binman/etype/fdtmap.py
> > index f1f6217940f2..a2b0d27fcc5f 100644
> > --- a/tools/binman/etype/fdtmap.py
> > +++ b/tools/binman/etype/fdtmap.py
> > @@ -109,6 +109,7 @@ class Entry_fdtmap(Entry):
> >          def _AddNode(node):
> >              """Add a node to the FDT map"""
> >              for pname, prop in node.props.items():
> > +                # pylint: disable=locally-disabled, possibly-used-before-assignment
> >                  fsw.property(pname, prop.bytes)
> >              for subnode in node.subnodes:
> >                  with fsw.add_node(subnode.name):
> > diff --git a/tools/binman/etype/image_header.py b/tools/binman/etype/image_header.py
> > index 240118849580..6d8db9eed564 100644
> > --- a/tools/binman/etype/image_header.py
> > +++ b/tools/binman/etype/image_header.py
> > @@ -72,6 +72,7 @@ class Entry_image_header(Entry):
> >              image_size = self.section.GetImageSize() or 0
> >              base = (0 if self.location != 'end' else image_size)
> >              offset = (image_pos - base) & 0xffffffff
> > +        # pylint: disable=locally-disabled, possibly-used-before-assignment
> >          data = IMAGE_HEADER_MAGIC + struct.pack('<I', offset)
> >          return data
> >
> > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> > index 2e4c72359ff3..e2615e1c7d1b 100644
> > --- a/tools/binman/etype/pre_load.py
> > +++ b/tools/binman/etype/pre_load.py
> > @@ -122,6 +122,7 @@ class Entry_pre_load(Entry_collection):
> >          else:
> >              self.Raise(padding_name + " is not supported")
> >
> > +        # pylint: disable=locally-disabled, possibly-used-before-assignment
> >          sig = padding.new(key, **padding_args).sign(hash_image)
> >
> >          hash_sig = SHA256.new()
> > diff --git a/tools/binman/etype/x509_cert.py b/tools/binman/etype/x509_cert.py
> > index 29630d1b86c8..15d0766051c2 100644
> > --- a/tools/binman/etype/x509_cert.py
> > +++ b/tools/binman/etype/x509_cert.py
> > @@ -141,6 +141,7 @@ class Entry_x509_cert(Entry_collection):
> >                  dm_data_ext_boot_block=self.dm_data_ext_boot_block,
> >                  bootcore_opts=self.bootcore_opts
> >              )
> > +        # pylint: disable=locally-disabled, possibly-used-before-assignment
> >          if stdout is not None:
> >              data = tools.read_file(output_fname)
> >          else:
> > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > index a553ca9e5642..b6e58decb008 100644
> > --- a/tools/binman/ftest.py
> > +++ b/tools/binman/ftest.py
> > @@ -6387,6 +6387,7 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> >                      expect_val = entry.image_pos
> >                  elif prop_name == 'size':
> >                      expect_val = entry.size
> > +                # pylint: disable=locally-disabled, possibly-used-before-assignment
> >                  self.assertEqual(expect_val, val)
> >
> >      def testSymbolsElfBad(self):
> > diff --git a/tools/binman/state.py b/tools/binman/state.py
> > index 45bae40c525a..ca3f857f1943 100644
> > --- a/tools/binman/state.py
> > +++ b/tools/binman/state.py
> > @@ -410,8 +410,8 @@ def CheckSetHashValue(node, get_data_func):
> >              m = hashlib.sha256()
> >              m.update(get_data_func())
> >              data = m.digest()
> > -        for n in GetUpdateNodes(hash_node):
> > -            n.SetData('value', data)
> > +            for n in GetUpdateNodes(hash_node):
> > +                n.SetData('value', data)
> >
> >  def SetAllowEntryExpansion(allow):
> >      """Set whether post-pack expansion of entries is allowed
>
> Any thoughts on this Simon? Moving to a newer Ubuntu host for CI is
> blocked on moving to a newer pylint. Thanks.

There is no need to disable these warnings so far as I can tell and it
sets a bad precedent. We should just fix the code. I can send a patch
for this if you like?

Regards,
Simon