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

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

Changes in v2:
- Safely handle non-NUL-terminated values. Uses strnlen() instead of
  strlen() to restrict accesses to the property value.
- Return FDT_ERR_BADVALUE if a non-NUL-terminated value is detected.
  Note that for performance reasons this doesn't always work. Cell or
  byte properties can look very much like NUL-terminated strings. The
  fdt_find_string() and fdt_get_string_index() functions can return a
  valid index or string, respectively, because they don't look at the
  complete value to determine validity. This could of course be fixed
  but the cases in which this succeeds the user did ask for trouble,
  so I didn't think it worth the extra effort.
- Improve documentation and tests.

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      |  92 ++++++++++++++++++++++++++++++
 libfdt/libfdt.h      |  82 ++++++++++++++++++++++++++-
 tests/.gitignore     |   1 +
 tests/Makefile.tests |   1 +
 tests/run_tests.sh   |   3 +
 tests/strings.c      | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/strings.dts    |  12 ++++
 7 files changed, 344 insertions(+), 1 deletion(-)
 create mode 100644 tests/strings.c
 create mode 100644 tests/strings.dts

-- 
2.4.5

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

This also adds a new error code, FDT_ERR_BADVALUE, that the function
returns when it encounters a non-NUL-terminated string list.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
- return FDT_ERR_BADVALUE error code for non-NUL-terminated properties
- add tests to cover non-NUL-terminated properties

 libfdt/fdt_ro.c      | 25 ++++++++++++++++
 libfdt/libfdt.h      | 19 +++++++++++-
 tests/.gitignore     |  1 +
 tests/Makefile.tests |  1 +
 tests/run_tests.sh   |  3 ++
 tests/strings.c      | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/strings.dts    | 12 ++++++++
 7 files changed, 142 insertions(+), 1 deletion(-)
 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..cf55c71df79c 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -538,6 +538,31 @@ 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)
+{
+	const char *list, *end;
+	int length, count = 0;
+
+	list = fdt_getprop(fdt, nodeoffset, property, &length);
+	if (!list)
+		return -length;
+
+	end = list + length;
+
+	while (list < end) {
+		length = strnlen(list, end - list) + 1;
+
+		/* Abort if the last string isn't properly NUL-terminated. */
+		if (list + length > end)
+			return -FDT_ERR_BADVALUE;
+
+		list += length;
+		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..bf60c1593e4e 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -121,7 +121,12 @@
 	/* FDT_ERR_BADNCELLS: Device tree has a #address-cells, #size-cells
 	 * or similar property with a bad format or value */
 
-#define FDT_ERR_MAX		14
+#define FDT_ERR_BADVALUE	15
+	/* FDT_ERR_BADVALUE: Device tree has a property with an unexpected
+	 * value. For example: a property expected to contain a string list
+	 * is not NUL-terminated within the length of its value. */
+
+#define FDT_ERR_MAX		15
 
 /**********************************************************************/
 /* Low-level functions (you probably don't need these)                */
@@ -868,6 +873,18 @@ 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
+ *   -FDT_ERR_BADVALUE if the property value is not NUL-terminated
+ *   -FDT_ERR_NOTFOUND if the property does not exist
+ */
+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..e10d9ece5a3e
--- /dev/null
+++ b/tests/strings.c
@@ -0,0 +1,82 @@
+/*
+ * 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_expected_failure(const void *fdt, const char *path,
+				   const char *property)
+{
+	int offset, err;
+
+	offset = fdt_path_offset(fdt, "/");
+	if (offset < 0)
+		FAIL("Couldn't find path %s", path);
+
+	err = fdt_count_strings(fdt, offset, "#address-cells");
+	if (err != -FDT_ERR_BADVALUE)
+		FAIL("unexpectedly succeeded in parsing #address-cells\n");
+}
+
+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("Couldn't count strings in property %s of node %s: %d\n",
+		     property, path, err);
+
+	if (err != count)
+		FAIL("String count for property %s of node %s is %d instead of %d\n",
+		     path, property, 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_expected_failure(fdt, "/", "#address-cells");
+	check_expected_failure(fdt, "/", "#size-cells");
+
+	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..1e4d3140458f
--- /dev/null
+++ b/tests/strings.dts
@@ -0,0 +1,12 @@
+/dts-v1/;
+
+/ {
+	compatible = "test-strings";
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	device {
+		compatible = "foo", "bar";
+		big-endian;
+	};
+};
-- 
2.4.5

[PATCH v2 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>
---
Changes in v2:
- handle non-NUL-terminated properties more gracefully
- improve documentation

 libfdt/fdt_ro.c | 30 ++++++++++++++++++++++++++++++
 libfdt/libfdt.h | 22 ++++++++++++++++++++++
 tests/strings.c | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index cf55c71df79c..39b84b1cea60 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -563,6 +563,36 @@ 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)
+{
+	int length, len, index = 0;
+	const char *list, *end;
+
+	list = fdt_getprop(fdt, nodeoffset, property, &length);
+	if (!list)
+		return -length;
+
+	len = strlen(string) + 1;
+	end = list + length;
+
+	while (list < end) {
+		length = strnlen(list, end - list) + 1;
+
+		/* Abort if the last string isn't properly NUL-terminated. */
+		if (list + length > end)
+			return -FDT_ERR_BADVALUE;
+
+		if (length == len && memcmp(list, string, length) == 0)
+			return index;
+
+		list += length;
+		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 bf60c1593e4e..e64680dd741c 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -885,6 +885,28 @@ 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
+ *
+ * Note that it is possible for this function to succeed on property values
+ * that are not NUL-terminated. That's because the function will stop after
+ * finding the first occurrence of @string. This can for example happen with
+ * small-valued cell properties, such as #address-cells, when searching for
+ * the empty string.
+ *
+ * @return:
+ *   the index of the string in the list of strings
+ *   -FDT_ERR_BADVALUE if the property value is not NUL-terminated
+ *   -FDT_ERR_NOTFOUND if the property does not exist or does not contain
+ *                     the given string
+ */
+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 e10d9ece5a3e..29c49bfcc78c 100644
--- a/tests/strings.c
+++ b/tests/strings.c
@@ -40,6 +40,24 @@ static void check_expected_failure(const void *fdt, const char *path,
 	err = fdt_count_strings(fdt, offset, "#address-cells");
 	if (err != -FDT_ERR_BADVALUE)
 		FAIL("unexpectedly succeeded in parsing #address-cells\n");
+
+	err = fdt_find_string(fdt, offset, "#address-cells", "foo");
+	if (err != -FDT_ERR_BADVALUE)
+		FAIL("found string in #address-cells: %d\n", err);
+
+	/*
+	 * Note that the #address-cells property contains a small 32-bit
+	 * unsigned integer, hence some bytes will be zero, and searching for
+	 * the empty string will succeed.
+	 *
+	 * The reason for this oddity is that the function will exit when the
+	 * first occurrence of the string is found, but in order to determine
+	 * that the property does not contain a valid string list it would
+	 * need to process the whole value.
+	 */
+	err = fdt_find_string(fdt, offset, "#address-cells", "");
+	if (err != 0)
+		FAIL("empty string not found in #address-cells: %d\n", err);
 }
 
 static void check_string_count(const void *fdt, const char *path,
@@ -61,6 +79,23 @@ static void check_string_count(const void *fdt, const char *path,
 		     path, property, 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 %s in property %s of node %s is %d, expected %d\n",
+		     string, property, path, err, index);
+}
+
 int main(int argc, char *argv[])
 {
 	void *fdt;
@@ -78,5 +113,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", "test-strings", 0);
+	check_string_index(fdt, "/device", "compatible", "foo", 0);
+	check_string_index(fdt, "/device", "compatible", "bar", 1);
+	check_string_index(fdt, "/device", "big-endian", "baz", -1);
+
 	PASS();
 }
-- 
2.4.5

[PATCH v2 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>
---
Changes in v2:
- handle non-NUL-terminated properties more gracefully
- improve documentation

 libfdt/fdt_ro.c | 37 +++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h | 41 +++++++++++++++++++++++++++++++++++++++++
 tests/strings.c | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 39b84b1cea60..4315815969b6 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -593,6 +593,43 @@ 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, *end;
+	int length;
+
+	list = fdt_getprop(fdt, nodeoffset, property, &length);
+	if (!list)
+		return -length;
+
+	end = list + length;
+
+	while (list < end) {
+		length = strnlen(list, end - list) + 1;
+
+		/* Abort if the last string isn't properly NUL-terminated. */
+		if (list + length > end)
+			return -FDT_ERR_BADVALUE;
+
+		if (index == 0) {
+			*output = list;
+			return 0;
+		}
+
+		list += length;
+		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 e64680dd741c..bfbc8f9ead34 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -907,6 +907,47 @@ 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
+ *
+ * Note that this will successfully extract strings from properties with
+ * non-NUL-terminated values. For example on small-valued cell properties
+ * this function will return the empty string.
+ *
+ * The @output parameter is only valid if the function returns 0.
+ *
+ * @return:
+ *   0 if the string was found (@output points to the string)
+ *   -FDT_ERR_BADVALUE if the property value is not NUL-terminated
+ *   -FDT_ERR_NOTFOUND if the property does not exist
+ */
+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
+ *
+ * This is a shorthand for:
+ *
+ *   fdt_get_string_index(fdt, node, property, 0, output).
+ *
+ * @return:
+ *   0 if the string was found (@output points to the string)
+ *   -FDT_ERR_BADVALUE if the property value is not NUL-terminated
+ *   -FDT_ERR_NOTFOUND if the property does not exist
+ */
+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 29c49bfcc78c..aa83ef0511be 100644
--- a/tests/strings.c
+++ b/tests/strings.c
@@ -58,6 +58,13 @@ static void check_expected_failure(const void *fdt, const char *path,
 	err = fdt_find_string(fdt, offset, "#address-cells", "");
 	if (err != 0)
 		FAIL("empty string not found in #address-cells: %d\n", err);
+
+	/*
+	 * fdt_get_string() can successfully extract strings from non-string
+	 * properties. This is because it doesn't necessarily parse the whole
+	 * property value, which would be necessary for it to determine if a
+	 * valid string or string list is present.
+	 */
 }
 
 static void check_string_count(const void *fdt, const char *path,
@@ -96,6 +103,27 @@ static void check_string_index(const void *fdt, const char *path,
 		     string, property, 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 from property %s of node %s: %d\n",
+		     index, property, path, err);
+
+	if (strcmp(string, result) != 0)
+		FAIL("String %d in property %s of node %s is %s, expected %s\n",
+		     index, property, path, result, string);
+}
+
 int main(int argc, char *argv[])
 {
 	void *fdt;
@@ -118,5 +146,9 @@ int main(int argc, char *argv[])
 	check_string_index(fdt, "/device", "compatible", "bar", 1);
 	check_string_index(fdt, "/device", "big-endian", "baz", -1);
 
+	check_string(fdt, "/", "compatible", 0, "test-strings");
+	check_string(fdt, "/device", "compatible", 0, "foo");
+	check_string(fdt, "/device", "compatible", 1, "bar");
+
 	PASS();
 }
-- 
2.4.5

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

[David Gibson]

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

On Thu, Jul 16, 2015 at 01:10:18PM +0200, Thierry Reding wrote:
> 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.
> 
> Changes in v2:
> - Safely handle non-NUL-terminated values. Uses strnlen() instead of
>   strlen() to restrict accesses to the property value.
> - Return FDT_ERR_BADVALUE if a non-NUL-terminated value is detected.
>   Note that for performance reasons this doesn't always work. Cell or
>   byte properties can look very much like NUL-terminated strings. The
>   fdt_find_string() and fdt_get_string_index() functions can return a
>   valid index or string, respectively, because they don't look at the
>   complete value to determine validity. This could of course be fixed
>   but the cases in which this succeeds the user did ask for trouble,
>   so I didn't think it worth the extra effort.
> - Improve documentation and tests.

Sorry, it looks like these fell off my radar for ages.

They look pretty good, but there are a few details I'm unhappy with.

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

[David Gibson]

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

On Thu, Jul 16, 2015 at 01:10:19PM +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.
> 
> This also adds a new error code, FDT_ERR_BADVALUE, that the function
> returns when it encounters a non-NUL-terminated string list.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v2:
> - return FDT_ERR_BADVALUE error code for non-NUL-terminated properties
> - add tests to cover non-NUL-terminated properties

This looks good, except for the function name.  I'd prefer to see it
called 'fdt_stringlist_count()' to match the existing
'fdt_stringlist_contains()'.

> 
>  libfdt/fdt_ro.c      | 25 ++++++++++++++++
>  libfdt/libfdt.h      | 19 +++++++++++-
>  tests/.gitignore     |  1 +
>  tests/Makefile.tests |  1 +
>  tests/run_tests.sh   |  3 ++
>  tests/strings.c      | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/strings.dts    | 12 ++++++++
>  7 files changed, 142 insertions(+), 1 deletion(-)
>  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..cf55c71df79c 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -538,6 +538,31 @@ 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)
> +{
> +	const char *list, *end;
> +	int length, count = 0;
> +
> +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> +	if (!list)
> +		return -length;
> +
> +	end = list + length;
> +
> +	while (list < end) {
> +		length = strnlen(list, end - list) + 1;
> +
> +		/* Abort if the last string isn't properly NUL-terminated. */
> +		if (list + length > end)
> +			return -FDT_ERR_BADVALUE;
> +
> +		list += length;
> +		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..bf60c1593e4e 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -121,7 +121,12 @@
>  	/* FDT_ERR_BADNCELLS: Device tree has a #address-cells, #size-cells
>  	 * or similar property with a bad format or value */
>  
> -#define FDT_ERR_MAX		14
> +#define FDT_ERR_BADVALUE	15
> +	/* FDT_ERR_BADVALUE: Device tree has a property with an unexpected
> +	 * value. For example: a property expected to contain a string list
> +	 * is not NUL-terminated within the length of its value. */
> +
> +#define FDT_ERR_MAX		15
>  
>  /**********************************************************************/
>  /* Low-level functions (you probably don't need these)                */
> @@ -868,6 +873,18 @@ 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
> + *   -FDT_ERR_BADVALUE if the property value is not NUL-terminated
> + *   -FDT_ERR_NOTFOUND if the property does not exist
> + */
> +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..e10d9ece5a3e
> --- /dev/null
> +++ b/tests/strings.c
> @@ -0,0 +1,82 @@
> +/*
> + * 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_expected_failure(const void *fdt, const char *path,
> +				   const char *property)
> +{
> +	int offset, err;
> +
> +	offset = fdt_path_offset(fdt, "/");
> +	if (offset < 0)
> +		FAIL("Couldn't find path %s", path);
> +
> +	err = fdt_count_strings(fdt, offset, "#address-cells");
> +	if (err != -FDT_ERR_BADVALUE)
> +		FAIL("unexpectedly succeeded in parsing #address-cells\n");
> +}
> +
> +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("Couldn't count strings in property %s of node %s: %d\n",
> +		     property, path, err);
> +
> +	if (err != count)
> +		FAIL("String count for property %s of node %s is %d instead of %d\n",
> +		     path, property, 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_expected_failure(fdt, "/", "#address-cells");
> +	check_expected_failure(fdt, "/", "#size-cells");
> +
> +	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..1e4d3140458f
> --- /dev/null
> +++ b/tests/strings.dts
> @@ -0,0 +1,12 @@
> +/dts-v1/;
> +
> +/ {
> +	compatible = "test-strings";
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	device {
> +		compatible = "foo", "bar";
> +		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 v2 2/3] fdt: Add a function to get the index of a string

[David Gibson]

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

On Thu, Jul 16, 2015 at 01:10:20PM +0200, Thierry Reding wrote:
> 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>

This also looks good, except for the name.  I'd prefer
'fdt_stringlist_search()' again to match the existing
'fdt_stringlist_contains()'.

Speaking of which, fdt_stringlist_contains() could be reimplemented in
terms of this function.

> ---
> Changes in v2:
> - handle non-NUL-terminated properties more gracefully
> - improve documentation
> 
>  libfdt/fdt_ro.c | 30 ++++++++++++++++++++++++++++++
>  libfdt/libfdt.h | 22 ++++++++++++++++++++++
>  tests/strings.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index cf55c71df79c..39b84b1cea60 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -563,6 +563,36 @@ 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)
> +{
> +	int length, len, index = 0;
> +	const char *list, *end;
> +
> +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> +	if (!list)
> +		return -length;
> +
> +	len = strlen(string) + 1;
> +	end = list + length;
> +
> +	while (list < end) {
> +		length = strnlen(list, end - list) + 1;
> +
> +		/* Abort if the last string isn't properly NUL-terminated. */
> +		if (list + length > end)
> +			return -FDT_ERR_BADVALUE;
> +
> +		if (length == len && memcmp(list, string, length) == 0)
> +			return index;
> +
> +		list += length;
> +		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 bf60c1593e4e..e64680dd741c 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -885,6 +885,28 @@ 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
> + *
> + * Note that it is possible for this function to succeed on property values
> + * that are not NUL-terminated. That's because the function will stop after
> + * finding the first occurrence of @string. This can for example happen with
> + * small-valued cell properties, such as #address-cells, when searching for
> + * the empty string.
> + *
> + * @return:
> + *   the index of the string in the list of strings
> + *   -FDT_ERR_BADVALUE if the property value is not NUL-terminated
> + *   -FDT_ERR_NOTFOUND if the property does not exist or does not contain
> + *                     the given string
> + */
> +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 e10d9ece5a3e..29c49bfcc78c 100644
> --- a/tests/strings.c
> +++ b/tests/strings.c
> @@ -40,6 +40,24 @@ static void check_expected_failure(const void *fdt, const char *path,
>  	err = fdt_count_strings(fdt, offset, "#address-cells");
>  	if (err != -FDT_ERR_BADVALUE)
>  		FAIL("unexpectedly succeeded in parsing #address-cells\n");
> +
> +	err = fdt_find_string(fdt, offset, "#address-cells", "foo");
> +	if (err != -FDT_ERR_BADVALUE)
> +		FAIL("found string in #address-cells: %d\n", err);
> +
> +	/*
> +	 * Note that the #address-cells property contains a small 32-bit
> +	 * unsigned integer, hence some bytes will be zero, and searching for
> +	 * the empty string will succeed.
> +	 *
> +	 * The reason for this oddity is that the function will exit when the
> +	 * first occurrence of the string is found, but in order to determine
> +	 * that the property does not contain a valid string list it would
> +	 * need to process the whole value.
> +	 */
> +	err = fdt_find_string(fdt, offset, "#address-cells", "");
> +	if (err != 0)
> +		FAIL("empty string not found in #address-cells: %d\n", err);
>  }
>  
>  static void check_string_count(const void *fdt, const char *path,
> @@ -61,6 +79,23 @@ static void check_string_count(const void *fdt, const char *path,
>  		     path, property, 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 %s in property %s of node %s is %d, expected %d\n",
> +		     string, property, path, err, index);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	void *fdt;
> @@ -78,5 +113,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", "test-strings", 0);
> +	check_string_index(fdt, "/device", "compatible", "foo", 0);
> +	check_string_index(fdt, "/device", "compatible", "bar", 1);
> +	check_string_index(fdt, "/device", "big-endian", "baz", -1);
> +
>  	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 --]

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

[David Gibson]

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

On Thu, Jul 16, 2015 at 01:10:21PM +0200, Thierry Reding wrote:
> 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>
> ---
> Changes in v2:
> - handle non-NUL-terminated properties more gracefully
> - improve documentation
> 
>  libfdt/fdt_ro.c | 37 +++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  tests/strings.c | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 110 insertions(+)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 39b84b1cea60..4315815969b6 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -593,6 +593,43 @@ 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)

So, once again, I don't like the name.  I'd prefer
'fdt_stringlist_get()'.

I'm also not 100% behind the interface.  In libfdt so far, we've
mostly avoided the pattern of returning just an error code, with
actual data returned via a pointer parameter.

It's still a bit ugly, but to closer match the signature of existing
functions like fdt_getprop_by_offset(), I'd prefer to see this return
the output string directly (or NULL on error).  Then add a *lenp
parameter which will return either the string's length or an error
code.

The function is already determining the string's length, and there's a
fair chance it could be useful to the caller.

> +{
> +	const char *list, *end;
> +	int length;
> +
> +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> +	if (!list)
> +		return -length;
> +
> +	end = list + length;
> +
> +	while (list < end) {
> +		length = strnlen(list, end - list) + 1;
> +
> +		/* Abort if the last string isn't properly NUL-terminated. */
> +		if (list + length > end)
> +			return -FDT_ERR_BADVALUE;
> +
> +		if (index == 0) {
> +			*output = list;
> +			return 0;
> +		}
> +
> +		list += length;
> +		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);
> +}
> +

Please drop this shortcut.  It's not that useful, and encourages a
caller to treat a stringlist property as if only the first string
matters, which is probably a bad idea.
-- 
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 v2 2/3] fdt: Add a function to get the index of a string

[Thierry Reding]

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

On Tue, Sep 29, 2015 at 03:54:46PM +1000, David Gibson wrote:
> On Thu, Jul 16, 2015 at 01:10:20PM +0200, Thierry Reding wrote:
> > 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>
> 
> This also looks good, except for the name.  I'd prefer
> 'fdt_stringlist_search()' again to match the existing
> 'fdt_stringlist_contains()'.

Done.

> Speaking of which, fdt_stringlist_contains() could be reimplemented in
> terms of this function.

Unfortunately it can't. fdt_stringlist_contains() doesn't operate on the
device tree node and property but rather on the raw stringlist itself.

Thierry

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

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

[Thierry Reding]

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

On Tue, Sep 29, 2015 at 03:53:13PM +1000, David Gibson wrote:
> On Thu, Jul 16, 2015 at 01:10:19PM +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.
> > 
> > This also adds a new error code, FDT_ERR_BADVALUE, that the function
> > returns when it encounters a non-NUL-terminated string list.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > Changes in v2:
> > - return FDT_ERR_BADVALUE error code for non-NUL-terminated properties
> > - add tests to cover non-NUL-terminated properties
> 
> This looks good, except for the function name.  I'd prefer to see it
> called 'fdt_stringlist_count()' to match the existing
> 'fdt_stringlist_contains()'.

Done.

Thierry

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

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

[Thierry Reding]

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

On Tue, Sep 29, 2015 at 04:00:41PM +1000, David Gibson wrote:
> On Thu, Jul 16, 2015 at 01:10:21PM +0200, Thierry Reding wrote:
> > 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>
> > ---
> > Changes in v2:
> > - handle non-NUL-terminated properties more gracefully
> > - improve documentation
> > 
> >  libfdt/fdt_ro.c | 37 +++++++++++++++++++++++++++++++++++++
> >  libfdt/libfdt.h | 41 +++++++++++++++++++++++++++++++++++++++++
> >  tests/strings.c | 32 ++++++++++++++++++++++++++++++++
> >  3 files changed, 110 insertions(+)
> > 
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index 39b84b1cea60..4315815969b6 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -593,6 +593,43 @@ 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)
> 
> So, once again, I don't like the name.  I'd prefer
> 'fdt_stringlist_get()'.
> 
> I'm also not 100% behind the interface.  In libfdt so far, we've
> mostly avoided the pattern of returning just an error code, with
> actual data returned via a pointer parameter.
> 
> It's still a bit ugly, but to closer match the signature of existing
> functions like fdt_getprop_by_offset(), I'd prefer to see this return
> the output string directly (or NULL on error).  Then add a *lenp
> parameter which will return either the string's length or an error
> code.
> 
> The function is already determining the string's length, and there's a
> fair chance it could be useful to the caller.

Sounds like a better interface. I've implemented these changes and sent
out a v3.

Thanks,
Thierry

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