On Tue, Feb 18, 2025 at 05:04:18PM -0700, Simon Glass wrote:
> Hi Tom,
>
> On Tue, 18 Feb 2025 at 12:25, Tom Rini <tr...@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 <tr...@konsulko.com>
> > > ---
> > > Cc: Simon Glass <s...@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?Yes, if you can fix the code please do as I don't see how to and they need to be disabled for pylint_err to pass with current pylint versions, thanks! -- Tom
signature.asc
Description: PGP signature
