On Thu, Jul 16, 2015 at 01:10:21PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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:
pgpbU6cjtuyUl.pgp
Description: PGP signature
