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 <tred...@nvidia.com>
> > > > >
> > > > > 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 <tred...@nvidia.com>
> > > > > ---
> > > > > 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
pgp3BhbBLELl3.pgp
Description: PGP signature
