Skip to contentSkip to conversation

libfdt/tests: Add fdt_path_offset_namelen() test

PATCH In review Peter Hurley 2 participants 5 messages Apr 5, 2015 – Apr 9, 2015 4 messages not in the archive

lore
Share & tools
lore

Peter Hurley

author
#0 lore raw

PATCH 1/1 libfdt/tests: Add fdt_path_offset_namelen() test

Add unit test for fdt_path_offset_namelen(). Verify partial path- descending retrieves the same node offset as fdt_subnode_offset(). Verify parsing correctness with multiple path separators, both mid-path and trailing.

Signed-off-by Peter Hurley <[email protected]>
---
 tests/Makefile.tests        |   2 +-
 tests/path_offset_namelen.c | 169 ++++++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh          |   1 +
 3 files changed, 171 insertions(+), 1 deletion(-)
 create mode 100644 tests/path_offset_namelen.c
tests/Makefile.tests +1 -1
index dafb618..9d5c456 100644
@@ -1,6 +1,6 @@
 LIB_TESTS_L = get_mem_rsv \
 	root_node find_property subnode_offset path_offset \
-	get_name getprop get_phandle \
+	path_offset_namelen get_name getprop get_phandle \
 	get_path supernode_atdepth_offset parent_offset \
 	node_offset_by_prop_value node_offset_by_phandle \
 	node_check_compatible node_offset_by_compatible \
tests/path_offset_namelen.c new file +169 -0

169 lines not on this page · raw message

tests/run_tests.sh +1 -0
index c0a136b..6c15c96 100755
@@ -143,6 +143,7 @@ tree1_tests () {
     run_test find_property $TREE
     run_test subnode_offset $TREE
     run_test path_offset $TREE
+    run_test path_offset_namelen $TREE
     run_test get_name $TREE
     run_test getprop $TREE
     run_test get_phandle $TREE
Signature · 6 lines
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1428206756-27160-1-git-send-email-peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org

#2 lore raw
Quoted 4 lines · Peter Hurley

Add unit test for fdt_path_offset_namelen(). Verify partial path- descending retrieves the same node offset as fdt_subnode_offset(). Verify parsing correctness with multiple path separators, both mid-path and trailing.

Thanks for writing this. However, looking at it, I find the way it's doing the path splitting a bit impenetrable, and overly tied to the expected usecase of fdt_path_offset_namelen() rather than just its defined semantics.

Instead of merging this, I've merged your original fdt_path_offset_namelen() patch, plus several patches which extend the existing path_offset testcase to exercise fdt_path_offset_namelen() and cover some other edge cases.

I did spot a case where the existing code is arguably incorrect: using fdt_path_offset_namelen(fdt, "/somenode\0foo", 13) will return the same as fdt_path_offset(fdt, "/somenode"), although I think ideally it should always return -FDT_ERR_NOTFOUND, or some other error, since node names can never include \0.

That's not an important enough problem to delay the patch though. So, fdt_path_offset_namelen() is merged and pushed up to the usual git tree on kernel.org.

Signature · 4 lines
David Gibson			| 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

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 10 lines · David Gibson
Quoted 4 lines · Peter Hurley

Add unit test for fdt_path_offset_namelen(). Verify partial path- descending retrieves the same node offset as fdt_subnode_offset(). Verify parsing correctness with multiple path separators, both mid-path and trailing.

Thanks for writing this. However, looking at it, I find the way it's doing the path splitting a bit impenetrable, and overly tied to the expected usecase of fdt_path_offset_namelen() rather than just its defined semantics.

Yeah; those are remnants of the unsubmitted v1, which retrieved the paths from the fdt itself, as I wrote in the other thread.

Quoted 4 lines

Instead of merging this, I've merged your original fdt_path_offset_namelen() patch, plus several patches which extend the existing path_offset testcase to exercise fdt_path_offset_namelen() and cover some other edge cases.

Ok, those look fine.

Quoted 5 lines

I did spot a case where the existing code is arguably incorrect: using fdt_path_offset_namelen(fdt, "/somenode\0foo", 13) will return the same as fdt_path_offset(fdt, "/somenode"), although I think ideally it should always return -FDT_ERR_NOTFOUND, or some other error, since node names can never include \0.

Ok, I'll fix that.

Quoted 3 lines

That's not an important enough problem to delay the patch though. So, fdt_path_offset_namelen() is merged and pushed up to the usual git tree on kernel.org.

Thanks.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 5523BBA7.8090504-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org

author
#6 lore raw
Quoted 8 lines · Peter Hurley
Quoted 5 lines · David Gibson

I did spot a case where the existing code is arguably incorrect: using fdt_path_offset_namelen(fdt, "/somenode\0foo", 13) will return the same as fdt_path_offset(fdt, "/somenode"), although I think ideally it should always return -FDT_ERR_NOTFOUND, or some other error, since node names can never include \0.

Ok, I'll fix that.

I think your memchr() is broken.

Patch below passes with the following output:
....
Checking offset of "/subnode@2/subsubnode@0/more" [first 24 characters] is 380... Checking offset of "/subnode@2/subsubnode@0/more" [first 25 characters] is -1... Checking offset of "/subnode@1" [first 14 characters] is -1...
PASS

Regards,
Peter Hurley

--- >% ---

tests/path_offset.c +2 -0
index bfebe9f..2788957 100644
@@ -133,5 +133,7 @@ int main(int argc, char *argv[])
 	check_path_offset_namelen(fdt, "/subnode@2/subsubnode@0/more", 24, subsubnode2_offset2);
 	check_path_offset_namelen(fdt, "/subnode@2/subsubnode@0/more", 25, -FDT_ERR_NOTFOUND);
 
+	check_path_offset_namelen(fdt, "/subnode@1\0foo", 14, -FDT_ERR_NOTFOUND);
+
 	PASS();
 }

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 5523C326.7010603-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org

#8 lore raw
Quoted 10 lines · Peter Hurley
Quoted 8 lines · Peter Hurley
Quoted 5 lines · David Gibson

I did spot a case where the existing code is arguably incorrect: using fdt_path_offset_namelen(fdt, "/somenode\0foo", 13) will return the same as fdt_path_offset(fdt, "/somenode"), although I think ideally it should always return -FDT_ERR_NOTFOUND, or some other error, since node names can never include \0.

Ok, I'll fix that.

Quoted 8 lines

I think your memchr() is broken.

Patch below passes with the following output:
....
Checking offset of "/subnode@2/subsubnode@0/more" [first 24 characters] is 380... Checking offset of "/subnode@2/subsubnode@0/more" [first 25 characters] is -1... Checking offset of "/subnode@1" [first 14 characters] is -1...
PASS

Huh, weird. I had a similar test to this, but not identical and the other day it failed. Today, your test passes for me, and I can't remember what my test was or otherwise trigger a problem.

Oh well, I guess we just leave it for now, and debug if it ever appears again.

Quoted 19 lines

Regards,
Peter Hurley

--- >% ---

tests/path_offset.c +2 -0
index bfebe9f..2788957 100644
@@ -133,5 +133,7 @@ int main(int argc, char *argv[])
 	check_path_offset_namelen(fdt, "/subnode@2/subsubnode@0/more", 24, subsubnode2_offset2);
 	check_path_offset_namelen(fdt, "/subnode@2/subsubnode@0/more", 25, -FDT_ERR_NOTFOUND);
 
+	check_path_offset_namelen(fdt, "/subnode@1\0foo", 14, -FDT_ERR_NOTFOUND);
+
 	PASS();
 }
Signature · 4 lines
David Gibson			| 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