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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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