[PATCH] libfdt/tests: Add fdt_path_offset_namelen() test

[PATCH] libfdt/tests: Add fdt_path_offset_namelen() test

[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.

Signed-off-by: Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>
---
 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

diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index dafb618..9d5c456 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -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 \
diff --git a/tests/path_offset_namelen.c b/tests/path_offset_namelen.c
new file mode 100644
index 0000000..3893d89
--- /dev/null
+++ b/tests/path_offset_namelen.c
@@ -0,0 +1,169 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for fdt_path_offset_namelen()
+ * Copyright (C) 2015 Peter Hurley
+ * based on path_offset.c, Copyright (C) 2006 David Gibson, IBM Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+static int check_subnode(void *fdt, int parent, const char *name)
+{
+	int offset;
+	const struct fdt_node_header *nh;
+	uint32_t tag;
+
+	verbose_printf("Checking subnode \"%s\" of %d...", name, parent);
+	offset = fdt_subnode_offset(fdt, parent, name);
+	verbose_printf("offset %d...", offset);
+	if (offset < 0)
+		FAIL("fdt_subnode_offset(\"%s\"): %s", name, fdt_strerror(offset));
+	nh = fdt_offset_ptr(fdt, offset, sizeof(*nh));
+	verbose_printf("pointer %p\n", nh);
+	if (! nh)
+		FAIL("NULL retrieving subnode \"%s\"", name);
+
+	tag = fdt32_to_cpu(nh->tag);
+
+	if (tag != FDT_BEGIN_NODE)
+		FAIL("Incorrect tag 0x%08x on property \"%s\"", tag, name);
+	if (!nodename_eq(nh->name, name))
+		FAIL("Subnode name mismatch \"%s\" instead of \"%s\"",
+		     nh->name, name);
+
+	return offset;
+}
+
+static void check_root(void *fdt)
+{
+	int root;
+
+	root = fdt_path_offset_namelen(fdt, "/", 1);
+	if (root < 0)
+		FAIL("fdt_path_offset_namelen(\"/\") failed: %s",
+		     fdt_strerror(root));
+	else if (root != 0)
+		FAIL("fdt_path_offset_namelen(\"/\") bad offset %d", root);
+}
+
+static void check_offsets(int subnode, int path)
+{
+	if (subnode != path)
+		FAIL("Mismatch: subnode_offset (%d) != path_offset (%d)",
+		     subnode, path);
+}
+
+static const char *get_subpath(const char *path, int len)
+{
+	const char *p;
+
+	p = strchr(path, '/');
+	if (!p)
+		FAIL("no / in %s", path);
+	p = strchr(p + 1, '/');
+	if (!p)
+		FAIL("no subpath %s", path);
+	if (p - path >= len)
+		FAIL("subpath len (%d) > path (%s) length (%d)",
+		     (int)(p - path), path, len);
+
+	return p;
+}
+
+static const char *get_subsubpath(const char *path, int len)
+{
+	const char *p;
+
+	p = strchr(path, ':');
+	if (!p)
+		FAIL("no subsubpath in %s", path);
+	if (p - path >= len)
+		FAIL("subsubpath len (%d) > path (%s) length (%d)",
+		     (int)(p - path), path, len);
+
+	return p;
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+	int subnode1_offset, subnode2_offset;
+	int subnode1_offset_p, subnode2_offset_p;
+	int subsubnode1_offset, subsubnode2_offset, subsubnode2_offset2;
+	int subsubnode1_offset_p, subsubnode2_offset_p, subsubnode2_offset2_p;
+	int path1_len, path2_len, path3_len, path4_len;
+	const char *p1, *p2, *p3, *p4;
+
+	static const char path1[] = "/subnode@1/subsubnode:";
+	static const char path2[] = "/subnode@2/subsubnode@0:";
+	static const char path3[] = "/subnode@1////subsubnode///:";
+	static const char path4[] = "/subnode@2///subsubnode///:/subnode@1";
+
+
+	test_init(argc, argv);
+	fdt = load_blob_arg(argc, argv);
+
+	check_root(fdt);
+
+	subnode1_offset = check_subnode(fdt, 0, "subnode@1");
+	subnode2_offset = check_subnode(fdt, 0, "subnode@2");
+	subsubnode1_offset = check_subnode(fdt, subnode1_offset, "subsubnode");
+	subsubnode2_offset = check_subnode(fdt, subnode2_offset, "subsubnode@0");
+	subsubnode2_offset2 = check_subnode(fdt, subnode2_offset, "subsubnode");
+
+	path1_len = strlen(path1);
+	path2_len = strlen(path2);
+	path3_len = strlen(path3);
+	path4_len = strlen(path4);
+
+	p1 = get_subpath(path1, path1_len);
+	p2 = get_subpath(path2, path2_len);
+	subnode1_offset_p = fdt_path_offset_namelen(fdt, path1, p1 - path1);
+	subnode2_offset_p = fdt_path_offset_namelen(fdt, path2, p2 - path2);
+	check_offsets(subnode1_offset, subnode1_offset_p);
+	check_offsets(subnode2_offset, subnode2_offset_p);
+
+	p1 = get_subsubpath(path1, path1_len);
+	p2 = get_subsubpath(path2, path2_len);
+	subsubnode1_offset_p = fdt_path_offset_namelen(fdt, path1, p1 - path1);
+	subsubnode2_offset_p = fdt_path_offset_namelen(fdt, path2, p2 - path2);
+	check_offsets(subsubnode1_offset, subsubnode1_offset_p);
+	check_offsets(subsubnode2_offset, subsubnode2_offset_p);
+
+	p3 = get_subpath(path3, path3_len);
+	p4 = get_subpath(path4, path4_len);
+	subnode1_offset_p = fdt_path_offset_namelen(fdt, path3, p3 - path3);
+	subnode2_offset_p = fdt_path_offset_namelen(fdt, path4, p4 - path4);
+	check_offsets(subnode1_offset, subnode1_offset_p);
+	check_offsets(subnode2_offset, subnode2_offset_p);
+
+	p3 = get_subsubpath(path3, path3_len);
+	p4 = get_subsubpath(path4, path4_len);
+	subsubnode1_offset_p = fdt_path_offset_namelen(fdt, path3, p3 - path3);
+	subsubnode2_offset2_p = fdt_path_offset_namelen(fdt, path4, p4 - path4);
+	check_offsets(subsubnode1_offset, subsubnode1_offset_p);
+	check_offsets(subsubnode2_offset2, subsubnode2_offset2_p);
+
+	PASS();
+}
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index c0a136b..6c15c96 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -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
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt/tests: Add fdt_path_offset_namelen() test

[David Gibson]

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

On Sun, Apr 05, 2015 at 12:05:56AM -0400, Peter Hurley wrote:
> 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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt/tests: Add fdt_path_offset_namelen() test

[Peter Hurley]

On 04/07/2015 12:49 AM, David Gibson wrote:
> On Sun, Apr 05, 2015 at 12:05:56AM -0400, Peter Hurley wrote:
>> 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.

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

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

> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt/tests: Add fdt_path_offset_namelen() test

[Peter Hurley]

On 04/07/2015 07:12 AM, Peter Hurley wrote:
> On 04/07/2015 12:49 AM, David Gibson wrote:
>> 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

--- >% ---
diff --git a/tests/path_offset.c b/tests/path_offset.c
index bfebe9f..2788957 100644
--- a/tests/path_offset.c
+++ b/tests/path_offset.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt/tests: Add fdt_path_offset_namelen() test

[David Gibson]

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

On Tue, Apr 07, 2015 at 07:44:38AM -0400, Peter Hurley wrote:
> On 04/07/2015 07:12 AM, Peter Hurley wrote:
> > On 04/07/2015 12:49 AM, David Gibson wrote:
> >> 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

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.

> 
> Regards,
> Peter Hurley
> 
> --- >% ---
> diff --git a/tests/path_offset.c b/tests/path_offset.c
> index bfebe9f..2788957 100644
> --- a/tests/path_offset.c
> +++ b/tests/path_offset.c
> @@ -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();
>  }
> 
> 

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]