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

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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