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

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

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