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

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

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

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@bootlin.com>
> ---
>  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.

>  	check_search(fdt, "subnode1", subnode1_offset, -FDT_ERR_NOTFOUND);
>  	check_search(fdt, "subsubnode1", subsubnode1_offset, -FDT_ERR_NOTFOUND);
>  	check_search(fdt, "subsubnode2", subsubnode2_offset, -FDT_ERR_NOTFOUND);
> diff --git a/tests/node_offset_by_prop_value.c b/tests/node_offset_by_prop_value.c
> index 48ab1d93..329409b9 100644
> --- a/tests/node_offset_by_prop_value.c
> +++ b/tests/node_offset_by_prop_value.c
> @@ -64,12 +64,14 @@ static void check_search_str(void *fdt, const char *propname,
>  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");
> @@ -79,19 +81,20 @@ int main(int argc, char *argv[])
>  	    || (subsubnode1_offset < 0) || (subsubnode2_offset < 0))
>  		FAIL("Can't find required nodes");
>  
> -	check_search_cell(fdt, "prop-int", TEST_VALUE_1, 0, subnode1_offset,
> -			  subsubnode1_offset, -FDT_ERR_NOTFOUND);
> +	check_search_cell(fdt, "prop-int", TEST_VALUE_1, root_offset,
> +			  subnode1_offset, subsubnode1_offset, -FDT_ERR_NOTFOUND);
>  
>  	check_search_cell(fdt, "prop-int", TEST_VALUE_2, subnode2_offset,
>  			  subsubnode2_offset, -FDT_ERR_NOTFOUND);
>  
> -	check_search_str(fdt, "prop-str", TEST_STRING_1, 0, -FDT_ERR_NOTFOUND);
> +	check_search_str(fdt, "prop-str", TEST_STRING_1, root_offset,
> +			  -FDT_ERR_NOTFOUND);
>  
>  	check_search_str(fdt, "prop-str", "no such string", -FDT_ERR_NOTFOUND);
>  
>  	check_search_cell(fdt, "prop-int", TEST_VALUE_1+1, -FDT_ERR_NOTFOUND);
>  
> -	check_search(fdt, "no-such-prop", NULL, 0, -FDT_ERR_NOTFOUND);
> +	check_search(fdt, "no-such-prop", NULL, root_offset, -FDT_ERR_NOTFOUND);
>  
>  	PASS();
>  }
> diff --git a/tests/path_offset.c b/tests/path_offset.c
> index ad8db833..d4f6553b 100644
> --- a/tests/path_offset.c
> +++ b/tests/path_offset.c
> @@ -84,11 +84,16 @@ int main(int argc, char *argv[])
>  	void *fdt;
>  	int subnode1_offset, subnode2_offset;
>  	int subsubnode1_offset, subsubnode2_offset, subsubnode2_offset2;
> +	int root_offset;
>  
>  	test_init(argc, argv);
>  	fdt = load_blob_arg(argc, argv);
>  
> -	check_path_offset(fdt, "/", 0);
> +	root_offset = fdt_root_offset(fdt);
> +	if (root_offset < 0)
> +		FAIL("fdt_root_offset()) failed: %s", fdt_strerror(root_offset));
> +
> +	check_path_offset(fdt, "/", root_offset);
>  
>  	subnode1_offset = check_subnode(fdt, 0, "subnode@1");
>  	subnode2_offset = check_subnode(fdt, 0, "subnode@2");
> @@ -106,8 +111,8 @@ int main(int argc, char *argv[])
>  
>  	/* Test paths with extraneous separators */
>  	check_path_offset(fdt, "", -FDT_ERR_BADPATH);
> -	check_path_offset(fdt, "//", 0);
> -	check_path_offset(fdt, "///", 0);
> +	check_path_offset(fdt, "//", root_offset);
> +	check_path_offset(fdt, "///", root_offset);
>  	check_path_offset(fdt, "//subnode@1", subnode1_offset);
>  	check_path_offset(fdt, "/subnode@1/", subnode1_offset);
>  	check_path_offset(fdt, "//subnode@1///", subnode1_offset);
> @@ -116,7 +121,7 @@ int main(int argc, char *argv[])
>  	/* Test fdt_path_offset_namelen() */
>  	check_path_offset_namelen(fdt, "/subnode@1", -1, -FDT_ERR_BADPATH);
>  	check_path_offset_namelen(fdt, "/subnode@1", 0, -FDT_ERR_BADPATH);
> -	check_path_offset_namelen(fdt, "/subnode@1", 1, 0);
> +	check_path_offset_namelen(fdt, "/subnode@1", 1, root_offset);
>  	check_path_offset_namelen(fdt, "/subnode@1/subsubnode", 10, subnode1_offset);
>  	check_path_offset_namelen(fdt, "/subnode@1/subsubnode", 11, subnode1_offset);
>  	check_path_offset_namelen(fdt, "/subnode@2TRAILINGGARBAGE", 10, subnode2_offset);
> 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.


>  
> +	nh = fdt_offset_ptr(fdt, root_offset, sizeof(*nh));
>  	if (! nh)
>  		FAIL("NULL retrieving root node");
>  
> -- 
> 2.55.0
> 
> 

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