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

[Doug Anderson]

Hi Simon,

On Fri, Oct 30, 2026 at 5:22 AM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
>
> > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > @@ -204,6 +204,23 @@ class FdtRo(object):
> > +    def subnodes(self, nodeoffset):
> > +        """Iterate over the subnodes of a node
> > +
> > +        Args:
> > +            nodeoffset: Offset of parent node to check
> > +
> > +        Yields:
> > +            Node offset of each direct subnode in order
> > +
> > +        Raises:
> > +            FdtException on error (e.g. invalid nodeoffset)
> > +        """
> > +        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. 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?

Done. Added a patch in v3 ("pylibfdt: Clarify iterator docstrings and test
negative offsets") noting in both `subnodes()` and `properties()` that the
tree must not be modified while iterating.


> > diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> > @@ -243,6 +243,21 @@ class PyLibfdtBasicTests(unittest.TestCase):
> > +        self.assertEqual(
> > +            [self.fdt.get_name(n) for n in self.fdt.subnodes(0)],
> > +            ['subnode@1', 'subnode@2'],
> > +        )
> > +        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.

Added the test for `list(self.fdt.properties(-1))` raising `FdtException`
(`-libfdt.BADOFFSET`) in `testPropertyOffsetExceptions`.

For `subnodes()`, it turns out `fdt_first_subnode()` and `fdt_next_subnode()`
in `libfdt/fdt.c` map all negative returns from `fdt_next_node()` to
`-FDT_ERR_NOTFOUND` (`if (offset < 0 || depth != 1) return -FDT_ERR_NOTFOUND;`,
and `fdt_next_node()` treats `offset < 0` as starting before the root node),
so `subnodes()` never raises `FdtException` on a bad offset. I removed the
inaccurate `Raises:` section from `subnodes()`'s docstring in the same
follow-up patch.


-Doug