Re: [PATCH v2 02/10] libfdt: Don't assume that a FDT_BEGIN_NODE tag is available at offset 0

--h6g3h+MAw4YSJRKZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 09, 2026 at 01:54:18PM +0200, Herve Codina wrote:
> In several places, libfdt assumes that a FDT_BEGIN_NODE tag is present
> at the offset 0 of the structure block.
>=20
> This assumption is not correct. Indeed, a FDT_NOP can be present at the
> offset 0 and this is a legit case.
>=20
> fdt_first_node() has been introduced recently to get the offset of the
> first node (first FDT_BEGIN_NODE) in a fdt blob.
>=20
> Use this function to get the first node offset instead of looking for
> this node at offset 0.
>=20
> Signed-off-by: Herve Codina <[email protected]>

The problem is real, of course.  But this approach to solving it with
a special case just for the root node is really ugly.

Granted, it's a problem of my own making - I chose not to create an
fdt_root_offset() function in the first place, instead making it part
of the API that offset 0 means the root node.  Nonetheless, here we
are and the question is whether we can do better.

# Straightforward things first

 - This patch should be folded with 1/10, they're both harder to
   understand without the context of the other.

 - If it must exist, the function should be fdt_root_offset(), not
   fdt_first_node(), for at least three reasons:
    * "first" in what sense?
    * "first" amongst what set of nodes?
    * We have a strong convention to always explicitly say "offset",
      not just referring to offset values as "node" or "property".
      This is deliberate: it's an attempt to discourage the otherwise
      likely misunderstanding that a function getting a "node" gives
      you some sort of persistent handle.  "offset" makes it clearer
      that the value will no longer be valid after a modification to
      the tree.

 - The situation described is subtle enough that this *really* needs a
   testcase.  It shouldn't be that hard: change the existing
   'nopulate' test tool to add an FDT_NOP before the first tag, not
   just after

# Is FDT_NOP before the root node actually legitimate?

Arguably the simplest solution here would be to explicitly ban this.
Yes, it would be a slightly odd restriction in the spec.  However,
avoiding the mess in the library might be worth it.  Note that this
situation can never arise from fdt_nop_node(), unless you apply it to
the root node, in which case there's no tree left.

# Less special casery

Even if we accept the need for FDT_NOP before the root node, I think
we can do better.  The below implements this as a special case, just
for offset 0.  Instead, we could allow all node operations on a
FDT_NOP offset, automatically advancing to the next FDT_BEGIN_NODE
tag.  We may be able to do that in check_node_offset_() minimising
code duplication.

--=20
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

--h6g3h+MAw4YSJRKZ
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmozxNcACgkQzQJF27ox
2GfTEA//Wo6xW1NH9oUsPfniRlezGMYCB1UQMgpfs4jWVgWTaOyAqrstX2tDZ6Km
xFF/SkAAdd5WGPnAiGdrrWPmPxbepreR5Gc5gSkbuSAH4HbJDS82h+0udM6PIb+n
xZvsb/0F5rk9Q9YzJlZmcLAoaIlhS6NxRG8mWAaQdnL4AyAS/KDD2BSDrLkld1N8
6/d7gkpRC7pr1UkwPNJACdHSYs3tBaayxKK7ZlNJZziUdvmI8r4LV6lornSrcyaQ
aoB4H0vSlrni1vvZyfpfAchlU98qKwcm1DlNmB0esuM/f2RUF4WHpEMWWDRhmQE5
ZH9NJUpWrQzCXOv3kn5MN16ePSYWnBOaFfyYNvL13kjsLaBjT58xNH5Wjxw2oP1F
f71Yoklw4yPmbYXsRBHgM0Km48N1g0rP7OAXmV5+jvae3UIUDGqVBMCh2kEpv4UR
1DYRfmCnc8aLoovsFxZmuIXBwqu6sRWfwCpRY/oKIN6gg0KBTIaa5W5KNLSIppHL
crzC8VNqipAQJukmrIoBfgbSdNrIp7JGKPHKGoAqA0yqpEwrryBowxO1Anf8NNua
LO0cKAPq8B8lV+yUs22BDc0R+KDiAEmfmzRMLwe3taUJluJPjnaHqbjPCWW2N32j
El6wd+bh7TckAEYaM1IM+9PpUORlawqtVHjUp0AnbaoORRIa+aY=
=jAFe
-----END PGP SIGNATURE-----

--h6g3h+MAw4YSJRKZ--