Re: [PATCH v2 03/10] pylibfdt: Add NodeTree live tree representation

[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
> > @@ -929,14 +929,16 @@ class Property(bytearray):
> >
> >      def as_str(self):
> >          """Unicode is supported by decoding from UTF-8"""
> > -        if self[-1] != 0:
> > +        if not self or self[-1] != 0:
> >              raise ValueError('Property lacks nul termination')
> >          if 0 in self[:-1]:
> >              raise ValueError('Property contains embedded nul characters')
> >          return self[:-1].decode('utf-8')
> >
> >      def as_stringlist(self):
> >          """Unicode is supported by decoding from UTF-8"""
> > +        if not self:
> > +            return []
> >          if self[-1] != 0:
> >              raise ValueError('Property lacks nul termination')
> >          parts = self[:-1].split(b'\x00')
>
> When you split this out (as David suggests), please can you add tests
> that call Property directly rather than only going through NodeTree?

Done. Split into its own patch ("pylibfdt: Handle empty properties in
as_str() and as_stringlist()") with direct tests on
`libfdt.Property('empty', b'')`.


> > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > @@ -1214,6 +1216,261 @@ class NodeAdder():
> > +class NodeTree:
>
> BTW in patch 7, fdtfactor adds 'phandle' and 'fixups' attributes to
> NodeTree objects on the fly, so they carry attributes the class does
> not declare. Since unflatten() uses cls and create_path() uses
> type(self), subclassing already works, so a small subclass in
> fdtfactor declaring these two attributes would be cleaner. What do you
> think?

Good idea! In v3 `fdtfactor` defines a `FactoredTree(devicetree.DeviceTree)`
subclass that initializes `self.phandle = None` and `self.fixups = {}` in
`__init__()`.


> > +        node = cls()
> > +        for prop in fdt.properties(offset):
> > +            node.props[prop.name] = bytes(prop)
> > +        for suboff in fdt.subnodes(offset):
> > +            node.children[fdt.get_name(suboff)] = cls.unflatten(fdt, suboff)
> > +        return node
>
> If a blob has two sibling nodes (or two properties) with the same
> name, one is silently dropped. I checked with FdtSw: with two 'a'
> subnodes under the root, only the second survives. libfdt does not
> reject such blobs and fdtfactor reads input it does not control, so I
> think it is better to raise ValueError here than to quietly throw data
> away.

Done. `DeviceTree.unflatten()` now raises `ValueError` if a node has
duplicate property names or duplicate sibling subnode names, with unit tests
in `tests/pylibfdt_tests.py`.


> > +        fdt = sw.as_fdt()
> > +        fdt_set_boot_cpuid_phys(fdt._fdt, boot_cpuid_phys)
> > +        fdt.pack()
> > +        return bytes(fdt.as_bytearray())
>
> This reaches into fdt._fdt and calls the raw SWIG function. It would
> be worth adding Fdt.set_boot_cpuid_phys() to go with the existing
> FdtRo.boot_cpuid_phys(), and calling that instead.

Done. Added `Fdt.set_boot_cpuid_phys()` in a preparatory patch and updated
`DeviceTree.flatten()` to call it.


-Doug