[PATCH 0/3] Add a couple of string-related functions

[PATCH 0/3] Add a couple of string-related functions

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

These three patches add a couple of string functions that have proven
useful in U-Boot's copy of libfdt, so they are likely to be useful for
other users as well.

Patch 1 adds a function to count the number of strings in a property's
value. This also adds a new DTS sample along with a small test program
to validate the implemented functions.

Patch 2 adds a function to retrieve the index of a given string in any
given property's value. This adds code to the test program introduced in
the previous patch to exercise the new functionality.

Patch 3 adds a function to retrieve a string by index from a property's
value along with a shortcut for index 0. This extends the test program
introduced in patch 1 to validate the new functionality.

Thierry

Thierry Reding (3):
  fdt: Add a function to count strings
  fdt: Add a function to get the index of a string
  fdt: Add functions to retrieve strings

 libfdt/fdt_ro.c      |  76 +++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h      |  47 ++++++++++++++++++++++
 tests/.gitignore     |   1 +
 tests/Makefile.tests |   1 +
 tests/run_tests.sh   |   3 ++
 tests/strings.c      | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/strings.dts    |  10 +++++
 7 files changed, 248 insertions(+)
 create mode 100644 tests/strings.c
 create mode 100644 tests/strings.dts

-- 
2.4.5

[PATCH 1/3] fdt: Add a function to count strings

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Given a device tree node and a property name, the fdt_count_strings()
function counts the number of strings found in the property value.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 libfdt/fdt_ro.c      | 20 ++++++++++++++++
 libfdt/libfdt.h      |  9 ++++++++
 tests/.gitignore     |  1 +
 tests/Makefile.tests |  1 +
 tests/run_tests.sh   |  3 +++
 tests/strings.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/strings.dts    | 10 ++++++++
 7 files changed, 108 insertions(+)
 create mode 100644 tests/strings.c
 create mode 100644 tests/strings.dts

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index a65e4b5b72b6..874975a0d8ad 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -538,6 +538,26 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
 	return 0;
 }
 
+int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
+{
+	int length, i, count = 0;
+	const char *list;
+
+	list = fdt_getprop(fdt, nodeoffset, property, &length);
+	if (!list)
+		return -length;
+
+	for (i = 0; i < length; i++) {
+		int len = strlen(list);
+
+		list += len + 1;
+		i += len;
+		count++;
+	}
+
+	return count;
+}
+
 int fdt_node_check_compatible(const void *fdt, int nodeoffset,
 			      const char *compatible)
 {
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 1054512814bc..93deec13655b 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -868,6 +868,15 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
  */
 int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
 
+/**
+ * fdt_count_strings - count the number of strings in a string list
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of a tree node
+ * @property: name of the property containing the string list
+ * @return: the number of strings in the given property
+ */
+int fdt_count_strings(const void *fdt, int nodeoffset, const char *property);
+
 /**********************************************************************/
 /* Read-only functions (addressing related)                           */
 /**********************************************************************/
diff --git a/tests/.gitignore b/tests/.gitignore
index 5656555f9cbc..09a1d3b84f6f 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -49,6 +49,7 @@ tmp.*
 /setprop_inplace
 /sized_cells
 /string_escapes
+/strings
 /subnode_iterate
 /subnode_offset
 /supernode_atdepth_offset
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index 9adedecdff3d..415e4d670f2c 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -9,6 +9,7 @@ LIB_TESTS_L = get_mem_rsv \
 	sized_cells \
 	notfound \
 	addr_size_cells \
+	strings \
 	setprop_inplace nop_property nop_node \
 	sw_tree1 \
 	move_and_save mangle-layout nopulate \
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 5268293434af..92ee96340c29 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -198,6 +198,9 @@ libfdt_tests () {
     run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
     run_test addr_size_cells addresses.test.dtb
 
+    run_dtc_test -I dts -O dtb -o strings.test.dtb strings.dts
+    run_test strings strings.test.dtb
+
     # Sequential write tests
     run_test sw_tree1
     tree1_tests sw_tree1.test.dtb
diff --git a/tests/strings.c b/tests/strings.c
new file mode 100644
index 000000000000..642057d1c8d9
--- /dev/null
+++ b/tests/strings.c
@@ -0,0 +1,64 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for string handling
+ * Copyright (C) 2015 NVIDIA 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 void check_string_count(const void *fdt, const char *path,
+			       const char *property, int count)
+{
+	int offset, err;
+
+	offset = fdt_path_offset(fdt, path);
+	if (offset < 0)
+		FAIL("Couldn't find path %s", path);
+
+	err = fdt_count_strings(fdt, offset, property);
+	if (err < 0)
+		FAIL("Failed to count strings: %d\n", err);
+
+	if (err != count)
+		FAIL("String count for %s is %d instead of %d\n",
+		     path, err, count);
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+
+	if (argc != 2)
+		CONFIG("Usage: %s <dtb file>\n", argv[0]);
+
+	test_init(argc, argv);
+	fdt = load_blob(argv[1]);
+
+	check_string_count(fdt, "/", "compatible", 1);
+	check_string_count(fdt, "/device", "compatible", 2);
+	check_string_count(fdt, "/device", "big-endian", 0);
+
+	PASS();
+}
diff --git a/tests/strings.dts b/tests/strings.dts
new file mode 100644
index 000000000000..04c2c202ec77
--- /dev/null
+++ b/tests/strings.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	compatible = "foo";
+
+	device {
+		compatible = "bar", "baz";
+		big-endian;
+	};
+};
-- 
2.4.5

[PATCH 2/3] fdt: Add a function to get the index of a string

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Given a device tree node and a property name, the new fdt_find_string()
function will look up a given string in the string list contained in the
property's value and return its index.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 libfdt/fdt_ro.c | 26 ++++++++++++++++++++++++++
 libfdt/libfdt.h | 11 +++++++++++
 tests/strings.c | 22 ++++++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 874975a0d8ad..aab3d1b94509 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -558,6 +558,32 @@ int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
 	return count;
 }
 
+int fdt_find_string(const void *fdt, int nodeoffset, const char *property,
+		    const char *string)
+{
+	const char *list, *end;
+	int len, index = 0;
+
+	list = fdt_getprop(fdt, nodeoffset, property, &len);
+	if (!list)
+		return len;
+
+	end = list + len;
+	len = strlen(string);
+
+	while (list < end) {
+		int l = strlen(list);
+
+		if (l == len && memcmp(list, string, len) == 0)
+			return index;
+
+		list += l + 1;
+		index++;
+	}
+
+	return -FDT_ERR_NOTFOUND;
+}
+
 int fdt_node_check_compatible(const void *fdt, int nodeoffset,
 			      const char *compatible)
 {
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 93deec13655b..34ab30dc3338 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -877,6 +877,17 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
  */
 int fdt_count_strings(const void *fdt, int nodeoffset, const char *property);
 
+/**
+ * fdt_find_string - find a string in a string list and return its index
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of a tree node
+ * @property: name of the property containing the string list
+ * @string: string to look up in the string list
+ * @return: the index of the string or negative on error
+ */
+int fdt_find_string(const void *fdt, int nodeoffset, const char *property,
+		    const char *string);
+
 /**********************************************************************/
 /* Read-only functions (addressing related)                           */
 /**********************************************************************/
diff --git a/tests/strings.c b/tests/strings.c
index 642057d1c8d9..050230567468 100644
--- a/tests/strings.c
+++ b/tests/strings.c
@@ -46,6 +46,23 @@ static void check_string_count(const void *fdt, const char *path,
 		     path, err, count);
 }
 
+static void check_string_index(const void *fdt, const char *path,
+			       const char *property, const char *string,
+			       int index)
+{
+	int offset, err;
+
+	offset = fdt_path_offset(fdt, path);
+	if (offset < 0)
+		FAIL("Couldn't find path %s", path);
+
+	err = fdt_find_string(fdt, offset, property, string);
+
+	if (err != index)
+		FAIL("Index of string %s is %d instead of %d\n",
+		     path, err, index);
+}
+
 int main(int argc, char *argv[])
 {
 	void *fdt;
@@ -60,5 +77,10 @@ int main(int argc, char *argv[])
 	check_string_count(fdt, "/device", "compatible", 2);
 	check_string_count(fdt, "/device", "big-endian", 0);
 
+	check_string_index(fdt, "/", "compatible", "foo", 0);
+	check_string_index(fdt, "/device", "compatible", "bar", 0);
+	check_string_index(fdt, "/device", "compatible", "baz", 1);
+	check_string_index(fdt, "/device", "big-endian", "bL", -1);
+
 	PASS();
 }
-- 
2.4.5

[PATCH 3/3] fdt: Add functions to retrieve strings

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Given a device tree node, a property name and an index, the new function
fdt_get_string_index() will return in an output argument a pointer to
the index'th string in the property's value.

The fdt_get_string() is a shortcut for the above with the index being 0.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 libfdt/fdt_ro.c | 30 ++++++++++++++++++++++++++++++
 libfdt/libfdt.h | 27 +++++++++++++++++++++++++++
 tests/strings.c | 24 ++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index aab3d1b94509..34f09d6c8c09 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -584,6 +584,36 @@ int fdt_find_string(const void *fdt, int nodeoffset, const char *property,
 	return -FDT_ERR_NOTFOUND;
 }
 
+int fdt_get_string_index(const void *fdt, int nodeoffset, const char *property,
+			 int index, const char **output)
+{
+	const char *list;
+	int length, i;
+
+	list = fdt_getprop(fdt, nodeoffset, property, &length);
+
+	for (i = 0; i < length; i++) {
+		int len = strlen(list);
+
+		if (index == 0) {
+			*output = list;
+			return 0;
+		}
+
+		list += len + 1;
+		i += len;
+		index--;
+	}
+
+	return FDT_ERR_NOTFOUND;
+}
+
+int fdt_get_string(const void *fdt, int nodeoffset, const char *property,
+		   const char **output)
+{
+	return fdt_get_string_index(fdt, nodeoffset, property, 0, output);
+}
+
 int fdt_node_check_compatible(const void *fdt, int nodeoffset,
 			      const char *compatible)
 {
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 34ab30dc3338..518550f1927d 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -888,6 +888,33 @@ int fdt_count_strings(const void *fdt, int nodeoffset, const char *property);
 int fdt_find_string(const void *fdt, int nodeoffset, const char *property,
 		    const char *string);
 
+/**
+ * fdt_get_string_index() - obtain the string at a given index in a string list
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of a tree node
+ * @property: name of the property containing the string list
+ * @index: index of the string to return
+ * @output: return location for the string
+ * @return: 0 if the string was found or a negative error code otherwise
+ */
+int fdt_get_string_index(const void *fdt, int nodeoffset, const char *property,
+			 int index, const char **output);
+
+/**
+ * fdt_get_string() - obtain the first string in a string list
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of a tree node
+ * @property: name of the property containing the string list
+ * @output: return location for the string
+ * @return: 0 if the string was found or a negative error code otherwise
+ *
+ * This is a shortcut for:
+ *
+ *	fdt_get_string_index(fdt, node, property, 0, output).
+ */
+int fdt_get_string(const void *fdt, int nodeoffset, const char *property,
+		   const char **output);
+
 /**********************************************************************/
 /* Read-only functions (addressing related)                           */
 /**********************************************************************/
diff --git a/tests/strings.c b/tests/strings.c
index 050230567468..e532e5fa8656 100644
--- a/tests/strings.c
+++ b/tests/strings.c
@@ -63,6 +63,26 @@ static void check_string_index(const void *fdt, const char *path,
 		     path, err, index);
 }
 
+static void check_string(const void *fdt, const char *path,
+			 const char *property, int index,
+			 const char *string)
+{
+	const char *result;
+	int offset, err;
+
+	offset = fdt_path_offset(fdt, path);
+	if (offset < 0)
+		FAIL("Couldn't find path %s", path);
+
+	err = fdt_get_string_index(fdt, offset, property, index, &result);
+	if (err < 0)
+		FAIL("Couldn't extract string: %d\n", err);
+
+	if (strcmp(string, result) != 0)
+		FAIL("String %d is %s instead of %s\n",
+		     index, result, string);
+}
+
 int main(int argc, char *argv[])
 {
 	void *fdt;
@@ -82,5 +102,9 @@ int main(int argc, char *argv[])
 	check_string_index(fdt, "/device", "compatible", "baz", 1);
 	check_string_index(fdt, "/device", "big-endian", "bL", -1);
 
+	check_string(fdt, "/", "compatible", 0, "foo");
+	check_string(fdt, "/device", "compatible", 0, "bar");
+	check_string(fdt, "/device", "compatible", 1, "baz");
+
 	PASS();
 }
-- 
2.4.5

Re: [PATCH 0/3] Add a couple of string-related functions

[Jon Loeliger]

So, like, Thierry Reding said:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> These three patches add a couple of string functions that have proven
> useful in U-Boot's copy of libfdt, so they are likely to be useful for
> other users as well.
> 
> Patch 1 adds a function to count the number of strings in a property's
> value. This also adds a new DTS sample along with a small test program
> to validate the implemented functions.
> 
> Patch 2 adds a function to retrieve the index of a given string in any
> given property's value. This adds code to the test program introduced in
> the previous patch to exercise the new functionality.
> 
> Patch 3 adds a function to retrieve a string by index from a property's
> value along with a shortcut for index 0. This extends the test program
> introduced in patch 1 to validate the new functionality.
> 
> Thierry


Hi Thierry,

While I am generally fine with this patch set, I have
a large-scope question.  Is there a larger plan to
consolidate or unify the U-Boot and DTC libfdts?

Thanks,
jdl

Re: [PATCH 1/3] fdt: Add a function to count strings

[David Gibson]

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

On Wed, Jul 15, 2015 at 01:13:57PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Given a device tree node and a property name, the fdt_count_strings()
> function counts the number of strings found in the property value.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  libfdt/fdt_ro.c      | 20 ++++++++++++++++
>  libfdt/libfdt.h      |  9 ++++++++
>  tests/.gitignore     |  1 +
>  tests/Makefile.tests |  1 +
>  tests/run_tests.sh   |  3 +++
>  tests/strings.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/strings.dts    | 10 ++++++++
>  7 files changed, 108 insertions(+)
>  create mode 100644 tests/strings.c
>  create mode 100644 tests/strings.dts
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index a65e4b5b72b6..874975a0d8ad 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -538,6 +538,26 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
>  	return 0;
>  }
>  
> +int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
> +{
> +	int length, i, count = 0;
> +	const char *list;
> +
> +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> +	if (!list)
> +		return -length;
> +
> +	for (i = 0; i < length; i++) {
> +		int len = strlen(list);

I like the concept of these patches, but this implementation is unsafe
if the given property does not, in fact, contain a list of \0
terminated strings.

> +		list += len + 1;
> +		i += len;
> +		count++;
> +	}
> +
> +	return count;
> +}
> +
>  int fdt_node_check_compatible(const void *fdt, int nodeoffset,
>  			      const char *compatible)
>  {
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 1054512814bc..93deec13655b 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -868,6 +868,15 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>   */
>  int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
>  
> +/**
> + * fdt_count_strings - count the number of strings in a string list
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of a tree node
> + * @property: name of the property containing the string list
> + * @return: the number of strings in the given property
> + */
> +int fdt_count_strings(const void *fdt, int nodeoffset, const char *property);
> +
>  /**********************************************************************/
>  /* Read-only functions (addressing related)                           */
>  /**********************************************************************/
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 5656555f9cbc..09a1d3b84f6f 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -49,6 +49,7 @@ tmp.*
>  /setprop_inplace
>  /sized_cells
>  /string_escapes
> +/strings
>  /subnode_iterate
>  /subnode_offset
>  /supernode_atdepth_offset
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 9adedecdff3d..415e4d670f2c 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -9,6 +9,7 @@ LIB_TESTS_L = get_mem_rsv \
>  	sized_cells \
>  	notfound \
>  	addr_size_cells \
> +	strings \
>  	setprop_inplace nop_property nop_node \
>  	sw_tree1 \
>  	move_and_save mangle-layout nopulate \
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 5268293434af..92ee96340c29 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -198,6 +198,9 @@ libfdt_tests () {
>      run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
>      run_test addr_size_cells addresses.test.dtb
>  
> +    run_dtc_test -I dts -O dtb -o strings.test.dtb strings.dts
> +    run_test strings strings.test.dtb
> +
>      # Sequential write tests
>      run_test sw_tree1
>      tree1_tests sw_tree1.test.dtb
> diff --git a/tests/strings.c b/tests/strings.c
> new file mode 100644
> index 000000000000..642057d1c8d9
> --- /dev/null
> +++ b/tests/strings.c
> @@ -0,0 +1,64 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + *	Testcase for string handling
> + * Copyright (C) 2015 NVIDIA 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 void check_string_count(const void *fdt, const char *path,
> +			       const char *property, int count)
> +{
> +	int offset, err;
> +
> +	offset = fdt_path_offset(fdt, path);
> +	if (offset < 0)
> +		FAIL("Couldn't find path %s", path);
> +
> +	err = fdt_count_strings(fdt, offset, property);
> +	if (err < 0)
> +		FAIL("Failed to count strings: %d\n", err);
> +
> +	if (err != count)
> +		FAIL("String count for %s is %d instead of %d\n",
> +		     path, err, count);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	void *fdt;
> +
> +	if (argc != 2)
> +		CONFIG("Usage: %s <dtb file>\n", argv[0]);
> +
> +	test_init(argc, argv);
> +	fdt = load_blob(argv[1]);
> +
> +	check_string_count(fdt, "/", "compatible", 1);
> +	check_string_count(fdt, "/device", "compatible", 2);
> +	check_string_count(fdt, "/device", "big-endian", 0);
> +
> +	PASS();
> +}
> diff --git a/tests/strings.dts b/tests/strings.dts
> new file mode 100644
> index 000000000000..04c2c202ec77
> --- /dev/null
> +++ b/tests/strings.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +
> +/ {
> +	compatible = "foo";
> +
> +	device {
> +		compatible = "bar", "baz";
> +		big-endian;
> +	};
> +};

-- 
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 0/3] Add a couple of string-related functions

[Simon Glass]

Hi Jon,

On 15 July 2015 at 07:29, Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org> wrote:
>
> So, like, Thierry Reding said:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > These three patches add a couple of string functions that have proven
> > useful in U-Boot's copy of libfdt, so they are likely to be useful for
> > other users as well.
> >
> > Patch 1 adds a function to count the number of strings in a property's
> > value. This also adds a new DTS sample along with a small test program
> > to validate the implemented functions.
> >
> > Patch 2 adds a function to retrieve the index of a given string in any
> > given property's value. This adds code to the test program introduced in
> > the previous patch to exercise the new functionality.
> >
> > Patch 3 adds a function to retrieve a string by index from a property's
> > value along with a shortcut for index 0. This extends the test program
> > introduced in patch 1 to validate the new functionality.
> >
> > Thierry
>
>
> Hi Thierry,
>
> While I am generally fine with this patch set, I have
> a large-scope question.  Is there a larger plan to
> consolidate or unify the U-Boot and DTC libfdts?

I maintain the fdt tree for U-Boot at present. About once a quarter I
check what has changed and do a bit of a sync. But there are things
that libfdt upstream has not accepted - e.g. the grep functionality
used by verified boot hashing stuff. I wish we could figure that out.
Perhaps a cut-down fdtgrep tool would meet with favour. We're using it
even more now since SPL (the minimal U-Boot loader) wants to run with
a subset of the full board FDT for SRAM space reasons.

I do ask people to send things upstream, and if rejected we then have
to work out what to do...there are recent U-Boot mailing list threads
on this.

Regards,
Simon

Re: [PATCH 0/3] Add a couple of string-related functions

[David Gibson]

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

On Wed, Jul 15, 2015 at 03:45:07PM -0600, Simon Glass wrote:
> Hi Jon,
> 
> On 15 July 2015 at 07:29, Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org> wrote:
> >
> > So, like, Thierry Reding said:
> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >
> > > These three patches add a couple of string functions that have proven
> > > useful in U-Boot's copy of libfdt, so they are likely to be useful for
> > > other users as well.
> > >
> > > Patch 1 adds a function to count the number of strings in a property's
> > > value. This also adds a new DTS sample along with a small test program
> > > to validate the implemented functions.
> > >
> > > Patch 2 adds a function to retrieve the index of a given string in any
> > > given property's value. This adds code to the test program introduced in
> > > the previous patch to exercise the new functionality.
> > >
> > > Patch 3 adds a function to retrieve a string by index from a property's
> > > value along with a shortcut for index 0. This extends the test program
> > > introduced in patch 1 to validate the new functionality.
> > >
> > > Thierry
> >
> >
> > Hi Thierry,
> >
> > While I am generally fine with this patch set, I have
> > a large-scope question.  Is there a larger plan to
> > consolidate or unify the U-Boot and DTC libfdts?
> 
> I maintain the fdt tree for U-Boot at present. About once a quarter I
> check what has changed and do a bit of a sync. But there are things
> that libfdt upstream has not accepted - e.g. the grep functionality
> used by verified boot hashing stuff. I wish we could figure that out.
> Perhaps a cut-down fdtgrep tool would meet with favour. We're using it
> even more now since SPL (the minimal U-Boot loader) wants to run with
> a subset of the full board FDT for SRAM space reasons.

So, short-term: there's no reason your fdtgrep stuff needs to be
considered as part of your version of libfdt - it could just as easily
be an add-on sitting alongside libfdt - then you could share the core
libfdt code at least.

Longer term, my main sticking point on the fdtgrep stuff was entry
points whose semantics don't make me go cross-eyed (includes these
nodes, but not those nodes, and might include children if this flag is
set, but not that one and the operator's shoe size matches some other
property...).  I'm not sure if that's a question of redesigning the
interface, or just of describing it better.

> I do ask people to send things upstream, and if rejected we then have
> to work out what to do...there are recent U-Boot mailing list threads
> on this.
> 
> Regards,
> Simon

-- 
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 1/3] fdt: Add a function to count strings

[Thierry Reding]

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

On Wed, Jul 15, 2015 at 11:41:30PM +1000, David Gibson wrote:
> On Wed, Jul 15, 2015 at 01:13:57PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Given a device tree node and a property name, the fdt_count_strings()
> > function counts the number of strings found in the property value.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  libfdt/fdt_ro.c      | 20 ++++++++++++++++
> >  libfdt/libfdt.h      |  9 ++++++++
> >  tests/.gitignore     |  1 +
> >  tests/Makefile.tests |  1 +
> >  tests/run_tests.sh   |  3 +++
> >  tests/strings.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/strings.dts    | 10 ++++++++
> >  7 files changed, 108 insertions(+)
> >  create mode 100644 tests/strings.c
> >  create mode 100644 tests/strings.dts
> > 
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index a65e4b5b72b6..874975a0d8ad 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -538,6 +538,26 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
> >  	return 0;
> >  }
> >  
> > +int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
> > +{
> > +	int length, i, count = 0;
> > +	const char *list;
> > +
> > +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> > +	if (!list)
> > +		return -length;
> > +
> > +	for (i = 0; i < length; i++) {
> > +		int len = strlen(list);
> 
> I like the concept of these patches, but this implementation is unsafe
> if the given property does not, in fact, contain a list of \0
> terminated strings.

This should be fixed in v2 of the patches. This isn't quite as simple as
using strnlen() instead of strlen() because it also means we need extra
handling for the case where a NUL byte isn't found within the value.

There is also a bit of an inconsistency for the fdt_find_string() and
fdt_get_string() functions because they can successfully work with non-
NUL-terminated values. I've gone into some detail about that in the v2
cover letter, so feedback there would be most welcome.

Thierry

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

Re: [PATCH 1/3] fdt: Add a function to count strings

[David Gibson]

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

On Thu, Jul 16, 2015 at 01:13:53PM +0200, Thierry Reding wrote:
> On Wed, Jul 15, 2015 at 11:41:30PM +1000, David Gibson wrote:
> > On Wed, Jul 15, 2015 at 01:13:57PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > 
> > > Given a device tree node and a property name, the fdt_count_strings()
> > > function counts the number of strings found in the property value.
> > > 
> > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  libfdt/fdt_ro.c      | 20 ++++++++++++++++
> > >  libfdt/libfdt.h      |  9 ++++++++
> > >  tests/.gitignore     |  1 +
> > >  tests/Makefile.tests |  1 +
> > >  tests/run_tests.sh   |  3 +++
> > >  tests/strings.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/strings.dts    | 10 ++++++++
> > >  7 files changed, 108 insertions(+)
> > >  create mode 100644 tests/strings.c
> > >  create mode 100644 tests/strings.dts
> > > 
> > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > index a65e4b5b72b6..874975a0d8ad 100644
> > > --- a/libfdt/fdt_ro.c
> > > +++ b/libfdt/fdt_ro.c
> > > @@ -538,6 +538,26 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
> > >  	return 0;
> > >  }
> > >  
> > > +int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
> > > +{
> > > +	int length, i, count = 0;
> > > +	const char *list;
> > > +
> > > +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> > > +	if (!list)
> > > +		return -length;
> > > +
> > > +	for (i = 0; i < length; i++) {
> > > +		int len = strlen(list);
> > 
> > I like the concept of these patches, but this implementation is unsafe
> > if the given property does not, in fact, contain a list of \0
> > terminated strings.
> 
> This should be fixed in v2 of the patches. This isn't quite as simple as
> using strnlen() instead of strlen() because it also means we need extra
> handling for the case where a NUL byte isn't found within the value.

Yes, I suspect it will actually be easier to use memchr().

> There is also a bit of an inconsistency for the fdt_find_string() and
> fdt_get_string() functions because they can successfully work with non-
> NUL-terminated values. I've gone into some detail about that in the v2
> cover letter, so feedback there would be most welcome.
> 
> Thierry



-- 
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 0/3] Add a couple of string-related functions

[Simon Glass]

Hi David,

On 15 July 2015 at 23:27, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Jul 15, 2015 at 03:45:07PM -0600, Simon Glass wrote:
>> Hi Jon,
>>
>> On 15 July 2015 at 07:29, Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org> wrote:
>> >
>> > So, like, Thierry Reding said:
>> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> > >
>> > > These three patches add a couple of string functions that have proven
>> > > useful in U-Boot's copy of libfdt, so they are likely to be useful for
>> > > other users as well.
>> > >
>> > > Patch 1 adds a function to count the number of strings in a property's
>> > > value. This also adds a new DTS sample along with a small test program
>> > > to validate the implemented functions.
>> > >
>> > > Patch 2 adds a function to retrieve the index of a given string in any
>> > > given property's value. This adds code to the test program introduced in
>> > > the previous patch to exercise the new functionality.
>> > >
>> > > Patch 3 adds a function to retrieve a string by index from a property's
>> > > value along with a shortcut for index 0. This extends the test program
>> > > introduced in patch 1 to validate the new functionality.
>> > >
>> > > Thierry
>> >
>> >
>> > Hi Thierry,
>> >
>> > While I am generally fine with this patch set, I have
>> > a large-scope question.  Is there a larger plan to
>> > consolidate or unify the U-Boot and DTC libfdts?
>>
>> I maintain the fdt tree for U-Boot at present. About once a quarter I
>> check what has changed and do a bit of a sync. But there are things
>> that libfdt upstream has not accepted - e.g. the grep functionality
>> used by verified boot hashing stuff. I wish we could figure that out.
>> Perhaps a cut-down fdtgrep tool would meet with favour. We're using it
>> even more now since SPL (the minimal U-Boot loader) wants to run with
>> a subset of the full board FDT for SRAM space reasons.
>
> So, short-term: there's no reason your fdtgrep stuff needs to be
> considered as part of your version of libfdt - it could just as easily
> be an add-on sitting alongside libfdt - then you could share the core
> libfdt code at least.

That's how it is today, yes.

>
> Longer term, my main sticking point on the fdtgrep stuff was entry
> points whose semantics don't make me go cross-eyed (includes these
> nodes, but not those nodes, and might include children if this flag is
> set, but not that one and the operator's shoe size matches some other
> property...).  I'm not sure if that's a question of redesigning the
> interface, or just of describing it better.

Neither am I, but perhaps if I cut down the fdtgrep options so that it
only does a few basic things that would help? The full feature set
would still be in the implementation, but it would reduce confusion on
that side.

>
>> I do ask people to send things upstream, and if rejected we then have
>> to work out what to do...there are recent U-Boot mailing list threads
>> on this.
>>
>> Regards,
>> Simon
>
> --
> 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

Regards,
Simon

Re: [PATCH 0/3] Add a couple of string-related functions

[David Gibson]

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

On Thu, Jul 16, 2015 at 10:56:23AM -0600, Simon Glass wrote:
> Hi David,
> 
> On 15 July 2015 at 23:27, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Jul 15, 2015 at 03:45:07PM -0600, Simon Glass wrote:
> >> Hi Jon,
> >>
> >> On 15 July 2015 at 07:29, Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org> wrote:
> >> >
> >> > So, like, Thierry Reding said:
> >> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> > >
> >> > > These three patches add a couple of string functions that have proven
> >> > > useful in U-Boot's copy of libfdt, so they are likely to be useful for
> >> > > other users as well.
> >> > >
> >> > > Patch 1 adds a function to count the number of strings in a property's
> >> > > value. This also adds a new DTS sample along with a small test program
> >> > > to validate the implemented functions.
> >> > >
> >> > > Patch 2 adds a function to retrieve the index of a given string in any
> >> > > given property's value. This adds code to the test program introduced in
> >> > > the previous patch to exercise the new functionality.
> >> > >
> >> > > Patch 3 adds a function to retrieve a string by index from a property's
> >> > > value along with a shortcut for index 0. This extends the test program
> >> > > introduced in patch 1 to validate the new functionality.
> >> > >
> >> > > Thierry
> >> >
> >> >
> >> > Hi Thierry,
> >> >
> >> > While I am generally fine with this patch set, I have
> >> > a large-scope question.  Is there a larger plan to
> >> > consolidate or unify the U-Boot and DTC libfdts?
> >>
> >> I maintain the fdt tree for U-Boot at present. About once a quarter I
> >> check what has changed and do a bit of a sync. But there are things
> >> that libfdt upstream has not accepted - e.g. the grep functionality
> >> used by verified boot hashing stuff. I wish we could figure that out.
> >> Perhaps a cut-down fdtgrep tool would meet with favour. We're using it
> >> even more now since SPL (the minimal U-Boot loader) wants to run with
> >> a subset of the full board FDT for SRAM space reasons.
> >
> > So, short-term: there's no reason your fdtgrep stuff needs to be
> > considered as part of your version of libfdt - it could just as easily
> > be an add-on sitting alongside libfdt - then you could share the core
> > libfdt code at least.
> 
> That's how it is today, yes.
> 
> >
> > Longer term, my main sticking point on the fdtgrep stuff was entry
> > points whose semantics don't make me go cross-eyed (includes these
> > nodes, but not those nodes, and might include children if this flag is
> > set, but not that one and the operator's shoe size matches some other
> > property...).  I'm not sure if that's a question of redesigning the
> > interface, or just of describing it better.
> 
> Neither am I, but perhaps if I cut down the fdtgrep options so that it
> only does a few basic things that would help? The full feature set
> would still be in the implementation, but it would reduce confusion on
> that side.

It's not really the fdtgrep tool which bothers me, I'm much more
concerned with the semantics of the libfdt function it uses to do its
work.

> >> I do ask people to send things upstream, and if rejected we then have
> >> to work out what to do...there are recent U-Boot mailing list threads
> >> on this.
> >>
> >> Regards,
> >> Simon
> >
> 
> Regards,
> Simon

-- 
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 1/3] fdt: Add a function to count strings

[Thierry Reding]

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

On Thu, Jul 16, 2015 at 10:29:35PM +1000, David Gibson wrote:
> On Thu, Jul 16, 2015 at 01:13:53PM +0200, Thierry Reding wrote:
> > On Wed, Jul 15, 2015 at 11:41:30PM +1000, David Gibson wrote:
> > > On Wed, Jul 15, 2015 at 01:13:57PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > 
> > > > Given a device tree node and a property name, the fdt_count_strings()
> > > > function counts the number of strings found in the property value.
> > > > 
> > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  libfdt/fdt_ro.c      | 20 ++++++++++++++++
> > > >  libfdt/libfdt.h      |  9 ++++++++
> > > >  tests/.gitignore     |  1 +
> > > >  tests/Makefile.tests |  1 +
> > > >  tests/run_tests.sh   |  3 +++
> > > >  tests/strings.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/strings.dts    | 10 ++++++++
> > > >  7 files changed, 108 insertions(+)
> > > >  create mode 100644 tests/strings.c
> > > >  create mode 100644 tests/strings.dts
> > > > 
> > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > index a65e4b5b72b6..874975a0d8ad 100644
> > > > --- a/libfdt/fdt_ro.c
> > > > +++ b/libfdt/fdt_ro.c
> > > > @@ -538,6 +538,26 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
> > > > +{
> > > > +	int length, i, count = 0;
> > > > +	const char *list;
> > > > +
> > > > +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> > > > +	if (!list)
> > > > +		return -length;
> > > > +
> > > > +	for (i = 0; i < length; i++) {
> > > > +		int len = strlen(list);
> > > 
> > > I like the concept of these patches, but this implementation is unsafe
> > > if the given property does not, in fact, contain a list of \0
> > > terminated strings.
> > 
> > This should be fixed in v2 of the patches. This isn't quite as simple as
> > using strnlen() instead of strlen() because it also means we need extra
> > handling for the case where a NUL byte isn't found within the value.
> 
> Yes, I suspect it will actually be easier to use memchr().

I don't see how that would make it easier. The problematic bit isn't
about determining whether or not the NUL byte is there. The problematic
bit is determining what to do about it. For example we could enforce
that these functions only work on well-defined properties, that is,
properties that end with a NUL-byte. Currently we don't do that in
fdt_find_string() and fdt_get_string() for performance reasons. For
fdt_count_strings() the check is implicit because the last string would
not be bounded by the property value if it wasn't terminated with a NUL
byte.

It would be easy to add this additional check as a way to short-circuit
implementations and abort early on malformed property values, but that
would be additional work that most users may not care about.

Thierry

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

Re: [PATCH 1/3] fdt: Add a function to count strings

[David Gibson]

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

On Fri, Jul 17, 2015 at 11:10:09AM +0200, Thierry Reding wrote:
> On Thu, Jul 16, 2015 at 10:29:35PM +1000, David Gibson wrote:
> > On Thu, Jul 16, 2015 at 01:13:53PM +0200, Thierry Reding wrote:
> > > On Wed, Jul 15, 2015 at 11:41:30PM +1000, David Gibson wrote:
> > > > On Wed, Jul 15, 2015 at 01:13:57PM +0200, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > > 
> > > > > Given a device tree node and a property name, the fdt_count_strings()
> > > > > function counts the number of strings found in the property value.
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > > ---
> > > > >  libfdt/fdt_ro.c      | 20 ++++++++++++++++
> > > > >  libfdt/libfdt.h      |  9 ++++++++
> > > > >  tests/.gitignore     |  1 +
> > > > >  tests/Makefile.tests |  1 +
> > > > >  tests/run_tests.sh   |  3 +++
> > > > >  tests/strings.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/strings.dts    | 10 ++++++++
> > > > >  7 files changed, 108 insertions(+)
> > > > >  create mode 100644 tests/strings.c
> > > > >  create mode 100644 tests/strings.dts
> > > > > 
> > > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > > index a65e4b5b72b6..874975a0d8ad 100644
> > > > > --- a/libfdt/fdt_ro.c
> > > > > +++ b/libfdt/fdt_ro.c
> > > > > @@ -538,6 +538,26 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
> > > > > +{
> > > > > +	int length, i, count = 0;
> > > > > +	const char *list;
> > > > > +
> > > > > +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> > > > > +	if (!list)
> > > > > +		return -length;
> > > > > +
> > > > > +	for (i = 0; i < length; i++) {
> > > > > +		int len = strlen(list);
> > > > 
> > > > I like the concept of these patches, but this implementation is unsafe
> > > > if the given property does not, in fact, contain a list of \0
> > > > terminated strings.
> > > 
> > > This should be fixed in v2 of the patches. This isn't quite as simple as
> > > using strnlen() instead of strlen() because it also means we need extra
> > > handling for the case where a NUL byte isn't found within the value.
> > 
> > Yes, I suspect it will actually be easier to use memchr().
> 
> I don't see how that would make it easier. The problematic bit isn't
> about determining whether or not the NUL byte is there. The problematic
> bit is determining what to do about it. For example we could enforce
> that these functions only work on well-defined properties, that is,
> properties that end with a NUL-byte. Currently we don't do that in
> fdt_find_string() and fdt_get_string() for performance reasons. For
> fdt_count_strings() the check is implicit because the last string would
> not be bounded by the property value if it wasn't terminated with a NUL
> byte.

Yeah, true; memchr() was just the first implementation that popped
into my head.

So, actually checking for a final \0 in advance isn't a real
performance cost - getprop already gives you the total property
length, so you can check if you have a valid stringlist in O(1).

> It would be easy to add this additional check as a way to short-circuit
> implementations and abort early on malformed property values, but that
> would be additional work that most users may not care about.

The implementation you have now seems fine to me - only thing I'm
still thinking about is whether I like the function names or not.

-- 
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 0/3] Add a couple of string-related functions

[Simon Glass]

Hi David,

On 16 July 2015 at 17:40, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Jul 16, 2015 at 10:56:23AM -0600, Simon Glass wrote:
>> Hi David,
>>
>> On 15 July 2015 at 23:27, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Wed, Jul 15, 2015 at 03:45:07PM -0600, Simon Glass wrote:
>> >> Hi Jon,
>> >>
>> >> On 15 July 2015 at 07:29, Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org> wrote:
>> >> >
>> >> > So, like, Thierry Reding said:
>> >> > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> >> > >
>> >> > > These three patches add a couple of string functions that have proven
>> >> > > useful in U-Boot's copy of libfdt, so they are likely to be useful for
>> >> > > other users as well.
>> >> > >
>> >> > > Patch 1 adds a function to count the number of strings in a property's
>> >> > > value. This also adds a new DTS sample along with a small test program
>> >> > > to validate the implemented functions.
>> >> > >
>> >> > > Patch 2 adds a function to retrieve the index of a given string in any
>> >> > > given property's value. This adds code to the test program introduced in
>> >> > > the previous patch to exercise the new functionality.
>> >> > >
>> >> > > Patch 3 adds a function to retrieve a string by index from a property's
>> >> > > value along with a shortcut for index 0. This extends the test program
>> >> > > introduced in patch 1 to validate the new functionality.
>> >> > >
>> >> > > Thierry
>> >> >
>> >> >
>> >> > Hi Thierry,
>> >> >
>> >> > While I am generally fine with this patch set, I have
>> >> > a large-scope question.  Is there a larger plan to
>> >> > consolidate or unify the U-Boot and DTC libfdts?
>> >>
>> >> I maintain the fdt tree for U-Boot at present. About once a quarter I
>> >> check what has changed and do a bit of a sync. But there are things
>> >> that libfdt upstream has not accepted - e.g. the grep functionality
>> >> used by verified boot hashing stuff. I wish we could figure that out.
>> >> Perhaps a cut-down fdtgrep tool would meet with favour. We're using it
>> >> even more now since SPL (the minimal U-Boot loader) wants to run with
>> >> a subset of the full board FDT for SRAM space reasons.
>> >
>> > So, short-term: there's no reason your fdtgrep stuff needs to be
>> > considered as part of your version of libfdt - it could just as easily
>> > be an add-on sitting alongside libfdt - then you could share the core
>> > libfdt code at least.
>>
>> That's how it is today, yes.
>>
>> >
>> > Longer term, my main sticking point on the fdtgrep stuff was entry
>> > points whose semantics don't make me go cross-eyed (includes these
>> > nodes, but not those nodes, and might include children if this flag is
>> > set, but not that one and the operator's shoe size matches some other
>> > property...).  I'm not sure if that's a question of redesigning the
>> > interface, or just of describing it better.
>>
>> Neither am I, but perhaps if I cut down the fdtgrep options so that it
>> only does a few basic things that would help? The full feature set
>> would still be in the implementation, but it would reduce confusion on
>> that side.
>
> It's not really the fdtgrep tool which bothers me, I'm much more
> concerned with the semantics of the libfdt function it uses to do its
> work.

Yes that's the tricky bit. I'll revisit it again with fresh eyes (it
has been a while) and see if there is something I can do.
Fundamentally it is not complex, but there are quite a few
combinations to deal with and I think that is the tricky part.

>
>> >> I do ask people to send things upstream, and if rejected we then have
>> >> to work out what to do...there are recent U-Boot mailing list threads
>> >> on this.
>> >>
>> >> Regards,
>> >> Simon
>> >
>>
>> Regards,
>> Simon

Regards,
Simon