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

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

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

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.
> 
> This assumption is not correct. Indeed, a FDT_NOP can be present at the
> offset 0 and this is a legit case.
> 
> fdt_first_node() has been introduced recently to get the offset of the
> first node (first FDT_BEGIN_NODE) in a fdt blob.
> 
> Use this function to get the first node offset instead of looking for
> this node at offset 0.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

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.

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