Re: [PATCH v2 02/10] pylibfdt: Add subnode/property iterators and get_mem_rsv_list()

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Oct 30, 2026 at 07:50:17AM -0500, Simon Glass wrote:
> Hi Doug,
> 
> On 2026-10-28T21:46:49, Douglas Anderson <dianders@chromium.org> wrote:
> > pylibfdt: Add subnode/property iterators and get_mem_rsv_list()
> >
> > Add Pythonic iterator helpers on `FdtRo` to simplify walking nodes,
> > properties, and memory reservation entries without manual offset loops
> > or `-FDT_ERR_NOTFOUND` sentinel handling:
> >
> > - `subnodes(nodeoffset)`: yields the offset of each direct child subnode
> >   of `nodeoffset`.
> > - `properties(nodeoffset)`: yields a `Property` object for each property
> >   of `nodeoffset`.
> > - `get_mem_rsv_list()`: returns a list of `(address, size)` tuples for
> >   all entries in the memory reservation block.
> >
> > Assisted-by: Gemini:gemini-pro-prerelease
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >
> >  pylibfdt/libfdt.i       | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/pylibfdt_tests.py | 18 ++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> 
> > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > @@ -201,6 +201,23 @@ class FdtRo(object):
> > +        soff = self.first_subnode(nodeoffset, quiet=QUIET_NOTFOUND)
> > +        while soff >= 0:
> > +            yield soff
> > +            soff = self.next_subnode(soff, quiet=QUIET_NOTFOUND)
> 
> Since Fdt inherits this generator, a caller can easily write 'for off
> in fdt.subnodes(0): fdt.del_node(off)' and get garbage, because the
> offsets move under it.

Right.  I noticed that, but merged anyway, since you can basically do
the same thing with the existing interfaces.  By abstracting it
further, this interface does arguably make the problem worse.  This is
kind of a perennial problem with the Python bindings.  The libfdt
interface is shaped by what is and isn't possible with the flattened
format - the reason we insistently name "offsets" everywhere, rather
than just "nodes" or "properties" is an attempt to inculcate the right
mental model.  By abstracting things more, the Python bindings make it
easier to miss that.

[Aside: one of the many projects I'll probably never find time for is
 to write a Rust-native libfdt, where the ownership model could be
 harnessed to enforce many of the required invariants]

> Since David has applied this, please can you
> send a follow-up noting in the docstring (here and in properties())
> that the tree must not be modified while iterating, as with
> fdt_for_each_subnode() in C?

Right, adding a comment is probably a good idea.

> > diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> > @@ -243,6 +243,22 @@ class PyLibfdtBasicTests(unittest.TestCase):
> > +        leaf = self.fdt.path_offset('/subnode@1/ss1')
> > +        self.assertEqual(list(self.fdt.subnodes(leaf)), [])
> 
> The docstrings say both iterators raise FdtException for a bad offset,
> but nothing tests that. A test that list(self.fdt.subnodes(-1)) and
> list(self.fdt.properties(-1)) raise, perhaps alongside
> testFirstNextSubnodeOffsetExceptions, would be good, perhaps in the
> same follow-up. Since these are generators, the exception only appears
> on the first next(), not when the method is called.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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