Re: [PATCH v3 03/15] tests: Don't assume the root node is available at offset 0

[David Gibson]

On Tue, Sep 01, 2026 at 03:36:26PM +0200, Herve Codina wrote:
> Hi David,
> 
> On Tue, 1 Sep 2026 18:03:09 +1000
> David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Wed, Aug 26, 2026 at 10:31:34AM +0200, Herve Codina wrote:
> > > Several tests uses offset 0 as the offset of the root node. Either to
> > > check the offset returned by tested functions or to directly manipulate
> > > the root node retrieved using fdt_offset_ptr(fdt, 0, ...).
> > > 
> > > The root node is not always at offset 0. Indeed, a FDT_NOP tag can be
> > > present at offset 0. fdt_root_offset() returns the offset of the root
> > > node taking care of possible FDT_NOP tag.
> > > 
> > > Use fdt_root_offset() to get the offset of the root node and use this
> > > value whenever the offset of the root node is expected.
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>
> > > ---
> > >  tests/node_offset_by_compatible.c |  4 +++-
> > >  tests/node_offset_by_prop_value.c | 11 +++++++----
> > >  tests/path_offset.c               | 13 +++++++++----
> > >  tests/root_node.c                 |  6 +++++-
> > >  4 files changed, 24 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/tests/node_offset_by_compatible.c b/tests/node_offset_by_compatible.c
> > > index a9e67835..1278a562 100644
> > > --- a/tests/node_offset_by_compatible.c
> > > +++ b/tests/node_offset_by_compatible.c
> > > @@ -39,12 +39,14 @@ static void check_search(void *fdt, const char *compat, ...)
> > >  int main(int argc, char *argv[])
> > >  {
> > >  	void *fdt;
> > > +	int root_offset;
> > >  	int subnode1_offset, subnode2_offset;
> > >  	int subsubnode1_offset, subsubnode2_offset;
> > >  
> > >  	test_init(argc, argv);
> > >  	fdt = load_blob_arg(argc, argv);
> > >  
> > > +	root_offset = fdt_root_offset(fdt);
> > >  	subnode1_offset = fdt_path_offset(fdt, "/subnode@1");
> > >  	subnode2_offset = fdt_path_offset(fdt, "/subnode@2");
> > >  	subsubnode1_offset = fdt_path_offset(fdt, "/subnode@1/subsubnode");
> > > @@ -54,7 +56,7 @@ int main(int argc, char *argv[])
> > >  	    || (subsubnode1_offset < 0) || (subsubnode2_offset < 0))
> > >  		FAIL("Can't find required nodes");
> > >  
> > > -	check_search(fdt, "test_tree1", 0, -FDT_ERR_NOTFOUND);
> > > +	check_search(fdt, "test_tree1", root_offset, -FDT_ERR_NOTFOUND);  
> > 
> > This does highlight that even with the compatibility changes
> > introduced here, allowing NOPs before the root node can potentially
> > break things.  We now handle _passing_ 0 to any of the functions as a
> > node offset, but anything that expects a _returned_ offset to be 0 if
> > it's the root node will break.
> > 
> > I think that's probably an acceptable breakage, but it's something to
> > be aware of.
> 
> Yes, I know but I wouldn't say it is a breakage. NOPs are allowed by the
> specification. In current version dtc/libfdt, having a NOP before the root
> node is already broken.

Yes, but (usually) not breaking existing users of real software is
more important than not breaking compliance with an abstract spec.
Especially when the spec was written from the software, not the other
way around.

> I agree with you we need to be aware of.
> 
> ...
> 
> > > diff --git a/tests/root_node.c b/tests/root_node.c
> > > index 37e6f059..30903f2b 100644
> > > --- a/tests/root_node.c
> > > +++ b/tests/root_node.c
> > > @@ -19,12 +19,16 @@ int main(int argc, char *argv[])
> > >  {
> > >  	void *fdt;
> > >  	const struct fdt_node_header *nh;
> > > +	int root_offset;
> > >  
> > >  	test_init(argc, argv);
> > >  	fdt = load_blob_arg(argc, argv);
> > >  
> > > -	nh = fdt_offset_ptr(fdt, 0, sizeof(*nh));
> > > +	root_offset = fdt_root_offset(fdt);
> > > +	if (root_offset < 0)
> > > +		FAIL("fdt_root_offset() returns %d", root_offset);  
> > 
> > Hm.  It's been a long time, but I suspect the purpose of this testcase
> > was to be super low-level, checking the contents of the root node
> > _without_ relying on iteration or lookup functions first.  Putting the
> > lookup call here arguably defeats that purpose: certainly the test
> > that nh->tag == FDT_BEGIN_NODE is no longer meaningful, since
> > fdt_root_offset() will explicitly look for a location where that's
> > true.
> > 
> > Or perhaps another way to look at it is that this test is explicitly
> > verifying that the root node is at offset 0, so does it make snese for
> > this test to even exist any more.  I guess the test for the name of
> > the root node is still meaningful, if minor.
> > 
> > Nonetheless, to maintain as best we can this test's goal as working
> > independent of lookup functions, I think it might be worth open coding
> > something to skip FDT_NOP tags (not using fdt_next_tag(), even), then
> > construct nh immediately after that.
> 
> Well "structured tags" are going to be introduced and so the open coding
> here will need to take care of that. Later some new tags could be introduced
> and could also have impacts on the test.

If structured tags are allowed before the root node.  There might be a
case for that (and maybe you've already made it in the addon series),
but so far it's not obvious to me that's useful.

> To be honest, I hesitated to just remove this root_node test.
> 
> I kept it because at least it checks that the offset returned by 
> fdt_root_offset() looks like a root node (i.e tag BEGIN_NODE and empty name).

empty name checks something, but the BEGIN_NODE test is meaningless if
we're found our offset by searching for a BEGIN_NODE tag.

> Adding an open coded loop here without fdt_next_tag() call will looks like
> a copy/paste of the loop available in fdtdump.c. I am not sure that this is
> what we want to see here.

Usually I'd agree (DRY principle).  But as I say, the purpose of this
test is a special case - it's trying to test things piece by piece
with a minimum of circular dependencies.  That tends to make it
clearer what's gone wrong when somethin gbreaks.

> The question is: How to be sure that the offset returned by fdt_root_offset()
> is really the root node offset without having an open code loop here?
> 
> Maybe we can add a new test, root_offset, and build dtbs where we exactly
> know where the root is (2 dtbs, with and without FDT_NOP). treegen could
> build those dtbs.
> ---- 8< ----
> / {
>     offset = <real_root_node_offset>;
> };
> ---- 8< ----
> 
> The treegen tool should be able to add a property where the value is the offset
> of the root node.
> 
> The new test could get the root node offset thanks to fdt_root_offset(), read the
> 'offset' property available at this root node offset and check that the property
> value matches the root node offset returned by fdt_root_offset().
> The root_node is kept as it is with this current patch applied:
> - use fdt_root_offset()
> - Check BEGIN_NODE
> - Check empty name
> 
> What do you think about this solution ?

One test that's safe on any tree just checking the empty name, another
that's checking the low-level construction with a known offset.
That's a good idea.

Encoding that know offset in a property is overcomplicating things
though, and it makes this very low-level test rely on parsing a
property, which we want to do even less that searching for the root
offset.  But that's ok - we're only calling it on specifically
constructed trees, so we can pass in the expected offset as an another
parameter.

> Should the 'root_offset' test need to be implemented or does it look
> over-engineered ?

No, it's good.  I'm less concerned about over-engineering in the
tests since, in a way, they test themselves.

-- 
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: signature.asc
Description: PGP signature