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

[PATCH v3 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. This extends the test program introduced in patch 1 to validate
the new functionality.

Changes in v3:
- Rename functions and rework prototypes to be more in line with the
  existing API.
- Remove shortcut for fdt_stringlist_get() with index 0 to discourage
  users from taking shortcuts when parsing lists of strings.

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

-- 
2.5.0

[PATCH v3 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_stringlist_count()
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 v3:
- rename function to fdt_stringlist_count()

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..4cde93193c92 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_stringlist_count(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 1bd024218b9c..2863001817c1 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_stringlist_count - 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_stringlist_count(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..923e2ed4de02
--- /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_stringlist_count(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_stringlist_count(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.5.0

[PATCH v3 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>

The new fdt_stringlist_search() function will look up a given string in
the list contained in the value of a named property of a given device
tree node and return its index.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- rename function to fdt_stringlist_search()

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 4cde93193c92..dae31732148a 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -563,6 +563,36 @@ int fdt_stringlist_count(const void *fdt, int nodeoffset, const char *property)
 	return count;
 }
 
+int fdt_stringlist_search(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 2863001817c1..aa376b83e7e5 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_stringlist_count(const void *fdt, int nodeoffset, const char *property);
 
+/**
+ * fdt_stringlist_search - 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_stringlist_search(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 923e2ed4de02..c8c7adb97051 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_stringlist_count(fdt, offset, "#address-cells");
 	if (err != -FDT_ERR_BADVALUE)
 		FAIL("unexpectedly succeeded in parsing #address-cells\n");
+
+	err = fdt_stringlist_search(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_stringlist_search(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_stringlist_search(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.5.0

[PATCH v3 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_stringlist_get() will return a pointer to the index'th string in the
property's value and return its length (or an error code on failure) in
an output argument.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- rename function to fdt_stringlist_get()
- return string (or NULL) instead of string length (or error code)
- return length of string or error code in output parameter
- remove non-indexed shortcut

Changes in v2:
- handle non-NUL-terminated properties more gracefully
- improve documentation

 libfdt/fdt_ro.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h | 28 ++++++++++++++++++++++++++++
 tests/strings.c | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index dae31732148a..c56ed8e826fe 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -593,6 +593,51 @@ int fdt_stringlist_search(const void *fdt, int nodeoffset, const char *property,
 	return -FDT_ERR_NOTFOUND;
 }
 
+const char *fdt_stringlist_get(const void *fdt, int nodeoffset,
+			       const char *property, int index,
+			       int *lenp)
+{
+	const char *list, *end;
+	int length;
+
+	list = fdt_getprop(fdt, nodeoffset, property, &length);
+	if (!list) {
+		if (lenp)
+			*lenp = length;
+
+		return NULL;
+	}
+
+	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) {
+			if (lenp)
+				*lenp = -FDT_ERR_BADVALUE;
+
+			return NULL;
+		}
+
+		if (index == 0) {
+			if (lenp)
+				*lenp = length - 1;
+
+			return list;
+		}
+
+		list += length;
+		index--;
+	}
+
+	if (lenp)
+		*lenp = -FDT_ERR_NOTFOUND;
+
+	return NULL;
+}
+
 int fdt_node_check_compatible(const void *fdt, int nodeoffset,
 			      const char *compatible)
 {
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index aa376b83e7e5..78adb1232e79 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -907,6 +907,34 @@ int fdt_stringlist_count(const void *fdt, int nodeoffset, const char *property);
 int fdt_stringlist_search(const void *fdt, int nodeoffset, const char *property,
 			  const char *string);
 
+/**
+ * fdt_stringlist_get() - 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
+ * @lenp: return location for the string length or an error code on failure
+ *
+ * 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.
+ *
+ * If non-NULL, the length of the string (on success) or a negative error-code
+ * (on failure) will be stored in the integer pointer to by lenp.
+ *
+ * @return:
+ *   A pointer to the string at the given index in the string list or NULL on
+ *   failure. On success the length of the string will be stored in the memory
+ *   location pointed to by the lenp parameter, if non-NULL. On failure one of
+ *   the following negative error codes will be returned in the lenp parameter
+ *   (if non-NULL):
+ *     -FDT_ERR_BADVALUE if the property value is not NUL-terminated
+ *     -FDT_ERR_NOTFOUND if the property does not exist
+ */
+const char *fdt_stringlist_get(const void *fdt, int nodeoffset,
+			       const char *property, int index,
+			       int *lenp);
+
 /**********************************************************************/
 /* Read-only functions (addressing related)                           */
 /**********************************************************************/
diff --git a/tests/strings.c b/tests/strings.c
index c8c7adb97051..8d23e965b58a 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_stringlist_search(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, len;
+
+	offset = fdt_path_offset(fdt, path);
+	if (offset < 0)
+		FAIL("Couldn't find path %s", path);
+
+	result = fdt_stringlist_get(fdt, offset, property, index, &len);
+	if (!result)
+		FAIL("Couldn't extract string %d from property %s of node %s: %d\n",
+		     index, property, path, len);
+
+	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.5.0

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

[David Gibson]

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

On Tue, Sep 29, 2015 at 11:09:05AM +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. This extends the test program introduced in patch 1 to validate
> the new functionality.

Thanks, I've applied this.

I did end up making a couple of small changes.

First, I forgot to say earlier that I also wanted to change the
testcase name to stringlist* for clarity.

Second, Travis CI picked up some -Wshadow warnings: in a bunch of
places you were using 'index' as a variable name, which shadows the
libc index(3) function.  I'm not sure why that didn't show up on a
local "make check", but in any case I've changed thos to 'idx' to fix
it.

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

[Thierry Reding]

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

On Wed, Sep 30, 2015 at 01:31:32PM +1000, David Gibson wrote:
> On Tue, Sep 29, 2015 at 11:09:05AM +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. This extends the test program introduced in patch 1 to validate
> > the new functionality.
> 
> Thanks, I've applied this.
> 
> I did end up making a couple of small changes.
> 
> First, I forgot to say earlier that I also wanted to change the
> testcase name to stringlist* for clarity.

Okay, makes sense.

> Second, Travis CI picked up some -Wshadow warnings: in a bunch of
> places you were using 'index' as a variable name, which shadows the
> libc index(3) function.  I'm not sure why that didn't show up on a
> local "make check", but in any case I've changed thos to 'idx' to fix
> it.

As far as I can tell that's because index(3) is declared in strings.h on
my system (the file ships with the GNU libc 2.22) and that header file
isn't included from anywhere. Perhaps your system differs from that?

Actually there's also a prototype for index(3) in string.h on my system
but it is guarded with an #ifdef __USE_MISC, and upon closer inspection
that does get set, so the index(3) symbol should be available. No idea
why I don't get a warning, though, -Wshadow is definitely getting passed
to the compiler.

Anyway, those changes sound reasonable in either case, thanks for
applying.

Thierry

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

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

[Thierry Reding]

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

On Thu, Oct 01, 2015 at 10:10:43AM +0200, Thierry Reding wrote:
> On Wed, Sep 30, 2015 at 01:31:32PM +1000, David Gibson wrote:
> > On Tue, Sep 29, 2015 at 11:09:05AM +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. This extends the test program introduced in patch 1 to validate
> > > the new functionality.
> > 
> > Thanks, I've applied this.
> > 
> > I did end up making a couple of small changes.
> > 
> > First, I forgot to say earlier that I also wanted to change the
> > testcase name to stringlist* for clarity.
> 
> Okay, makes sense.
> 
> > Second, Travis CI picked up some -Wshadow warnings: in a bunch of
> > places you were using 'index' as a variable name, which shadows the
> > libc index(3) function.  I'm not sure why that didn't show up on a
> > local "make check", but in any case I've changed thos to 'idx' to fix
> > it.
> 
> As far as I can tell that's because index(3) is declared in strings.h on
> my system (the file ships with the GNU libc 2.22) and that header file
> isn't included from anywhere. Perhaps your system differs from that?
> 
> Actually there's also a prototype for index(3) in string.h on my system
> but it is guarded with an #ifdef __USE_MISC, and upon closer inspection
> that does get set, so the index(3) symbol should be available. No idea
> why I don't get a warning, though, -Wshadow is definitely getting passed
> to the compiler.

Ah, some further digging shows that this behaviour was changed in GCC
4.8 (search for -Wshadow):

and the link in that note links to a note from Linus Torvalds here:

	https://lkml.org/lkml/2006/11/28/239

which I find quite amusing because the reason for the change in GCC 4.8
was exactly the same example of using a local variable named index while
including string.h.

I suspect that you're using some version of GCC older than 4.8, which
would explain why you saw the warning and I didn't (I use 5.2.0).

Thierry

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

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

[Thierry Reding]

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

On Thu, Oct 01, 2015 at 10:23:42AM +0200, Thierry Reding wrote:
> On Thu, Oct 01, 2015 at 10:10:43AM +0200, Thierry Reding wrote:
> > On Wed, Sep 30, 2015 at 01:31:32PM +1000, David Gibson wrote:
> > > On Tue, Sep 29, 2015 at 11:09:05AM +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. This extends the test program introduced in patch 1 to validate
> > > > the new functionality.
> > > 
> > > Thanks, I've applied this.
> > > 
> > > I did end up making a couple of small changes.
> > > 
> > > First, I forgot to say earlier that I also wanted to change the
> > > testcase name to stringlist* for clarity.
> > 
> > Okay, makes sense.
> > 
> > > Second, Travis CI picked up some -Wshadow warnings: in a bunch of
> > > places you were using 'index' as a variable name, which shadows the
> > > libc index(3) function.  I'm not sure why that didn't show up on a
> > > local "make check", but in any case I've changed thos to 'idx' to fix
> > > it.
> > 
> > As far as I can tell that's because index(3) is declared in strings.h on
> > my system (the file ships with the GNU libc 2.22) and that header file
> > isn't included from anywhere. Perhaps your system differs from that?
> > 
> > Actually there's also a prototype for index(3) in string.h on my system
> > but it is guarded with an #ifdef __USE_MISC, and upon closer inspection
> > that does get set, so the index(3) symbol should be available. No idea
> > why I don't get a warning, though, -Wshadow is definitely getting passed
> > to the compiler.
> 
> Ah, some further digging shows that this behaviour was changed in GCC
> 4.8 (search for -Wshadow):

Oops, there should have been a link here:

	https://gcc.gnu.org/gcc-4.8/changes.html

Thierry

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

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

[David Gibson]

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

On Thu, Oct 01, 2015 at 10:23:42AM +0200, Thierry Reding wrote:
> On Thu, Oct 01, 2015 at 10:10:43AM +0200, Thierry Reding wrote:
> > On Wed, Sep 30, 2015 at 01:31:32PM +1000, David Gibson wrote:
> > > On Tue, Sep 29, 2015 at 11:09:05AM +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. This extends the test program introduced in patch 1 to validate
> > > > the new functionality.
> > > 
> > > Thanks, I've applied this.
> > > 
> > > I did end up making a couple of small changes.
> > > 
> > > First, I forgot to say earlier that I also wanted to change the
> > > testcase name to stringlist* for clarity.
> > 
> > Okay, makes sense.
> > 
> > > Second, Travis CI picked up some -Wshadow warnings: in a bunch of
> > > places you were using 'index' as a variable name, which shadows the
> > > libc index(3) function.  I'm not sure why that didn't show up on a
> > > local "make check", but in any case I've changed thos to 'idx' to fix
> > > it.
> > 
> > As far as I can tell that's because index(3) is declared in strings.h on
> > my system (the file ships with the GNU libc 2.22) and that header file
> > isn't included from anywhere. Perhaps your system differs from that?

No, it doesn't show up on my own system, just on the Travis CI system
(which is really useful, btw).  See

https://travis-ci.org/dgibson/dtc/builds/82862264 

> > Actually there's also a prototype for index(3) in string.h on my system
> > but it is guarded with an #ifdef __USE_MISC, and upon closer inspection
> > that does get set, so the index(3) symbol should be available. No idea
> > why I don't get a warning, though, -Wshadow is definitely getting passed
> > to the compiler.
> 
> Ah, some further digging shows that this behaviour was changed in GCC
> 4.8 (search for -Wshadow):

Ah, that'd do it.  Travis is apparently using gcc 4.6.  I have 5.1.1
on my own machine.

> and the link in that note links to a note from Linus Torvalds here:
> 
> 	https://lkml.org/lkml/2006/11/28/239
> 
> which I find quite amusing because the reason for the change in GCC 4.8
> was exactly the same example of using a local variable named index while
> including string.h.
> 
> I suspect that you're using some version of GCC older than 4.8, which
> would explain why you saw the warning and I didn't (I use 5.2.0).
> 
> 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 --]