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

[Simon Glass]

Hi David,

On Sun, 1 Nov 2026 at 18:49, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Oct 30, 2026 at 07:50:17AM -0500, Simon Glass wrote:
> > Hi Doug,
> >
> > On 2026-10-28T21:46:49, Douglas Anderson <dianders@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > >
> > >  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.

Yes that makes sense, and noted.

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

Yes we talked about that briefly last year I think. I haven't
seriously looked at it though, apart from some experiments with
ownership of an offset. I have dug into Rust quite a bit but I'm sure
there are other people who would do a better job of this.

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

Regards,
Simon