Re: [PATCH 1/3] fdt: Add a function to count strings

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

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

On Fri, Jul 17, 2015 at 11:10:09AM +0200, Thierry Reding wrote:
> On Thu, Jul 16, 2015 at 10:29:35PM +1000, David Gibson wrote:
> > On Thu, Jul 16, 2015 at 01:13:53PM +0200, Thierry Reding wrote:
> > > On Wed, Jul 15, 2015 at 11:41:30PM +1000, David Gibson wrote:
> > > > On Wed, Jul 15, 2015 at 01:13:57PM +0200, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > > 
> > > > > Given a device tree node and a property name, the fdt_count_strings()
> > > > > function counts the number of strings found in the property value.
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > > ---
> > > > >  libfdt/fdt_ro.c      | 20 ++++++++++++++++
> > > > >  libfdt/libfdt.h      |  9 ++++++++
> > > > >  tests/.gitignore     |  1 +
> > > > >  tests/Makefile.tests |  1 +
> > > > >  tests/run_tests.sh   |  3 +++
> > > > >  tests/strings.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/strings.dts    | 10 ++++++++
> > > > >  7 files changed, 108 insertions(+)
> > > > >  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..874975a0d8ad 100644
> > > > > --- a/libfdt/fdt_ro.c
> > > > > +++ b/libfdt/fdt_ro.c
> > > > > @@ -538,6 +538,26 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +int fdt_count_strings(const void *fdt, int nodeoffset, const char *property)
> > > > > +{
> > > > > +	int length, i, count = 0;
> > > > > +	const char *list;
> > > > > +
> > > > > +	list = fdt_getprop(fdt, nodeoffset, property, &length);
> > > > > +	if (!list)
> > > > > +		return -length;
> > > > > +
> > > > > +	for (i = 0; i < length; i++) {
> > > > > +		int len = strlen(list);
> > > > 
> > > > I like the concept of these patches, but this implementation is unsafe
> > > > if the given property does not, in fact, contain a list of \0
> > > > terminated strings.
> > > 
> > > This should be fixed in v2 of the patches. This isn't quite as simple as
> > > using strnlen() instead of strlen() because it also means we need extra
> > > handling for the case where a NUL byte isn't found within the value.
> > 
> > Yes, I suspect it will actually be easier to use memchr().
> 
> I don't see how that would make it easier. The problematic bit isn't
> about determining whether or not the NUL byte is there. The problematic
> bit is determining what to do about it. For example we could enforce
> that these functions only work on well-defined properties, that is,
> properties that end with a NUL-byte. Currently we don't do that in
> fdt_find_string() and fdt_get_string() for performance reasons. For
> fdt_count_strings() the check is implicit because the last string would
> not be bounded by the property value if it wasn't terminated with a NUL
> byte.

Yeah, true; memchr() was just the first implementation that popped
into my head.

So, actually checking for a final \0 in advance isn't a real
performance cost - getprop already gives you the total property
length, so you can check if you have a valid stringlist in O(1).

> It would be easy to add this additional check as a way to short-circuit
> implementations and abort early on malformed property values, but that
> would be additional work that most users may not care about.

The implementation you have now seems fine to me - only thing I'm
still thinking about is whether I like the function names or not.

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