[PATCH] libfdt: fdt_path_offset_*(): Fix handling of paths with options in them

[PATCH] libfdt: fdt_path_offset_*(): Fix handling of paths with options in them

[Hans de Goede]

This is another approach at fixing the issues with paths with have options
appended seperated by a ':' character.

commit b4150b59ae ("libfdt: Add fdt_path_offset_namelen()")

Is related to this, it allows the caller to specify to only look at part
of the passed in path. But as experience with using this in the kernel has
shown using this properly is quite hard since the options itself may have
a '/' in them, also see the comment above the new fdt_path_next_seperator
helper this commit adds.

So this commit, which currently is being used by u-boot, instead simply
teaches fdt_path_offset() to just do the right thing when it encounters
paths with a ':' in them.

Cc: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 libfdt/fdt_ro.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index a65e4b5..9efbcb2 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -154,6 +154,25 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
 	return fdt_subnode_offset_namelen(fdt, parentoffset, name, strlen(name));
 }
 
+/*
+ * Find the next of path seperator, note we need to search for both '/' and ':'
+ * and then take the first one so that we do the rigth thing for e.g.
+ * "foo/bar:option" and "bar:option/otheroption", both of which happen, so
+ * first searching for either ':' or '/' does not work.
+ */
+static const char *fdt_path_next_seperator(const char *path, int len)
+{
+	const char *sep1 = memchr(path, '/', len);
+	const char *sep2 = memchr(path, ':', len);
+
+	if (sep1 && sep2)
+		return (sep1 < sep2) ? sep1 : sep2;
+	else if (sep1)
+		return sep1;
+	else
+		return sep2;
+}
+
 int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
 {
 	const char *end = path + namelen;
@@ -164,7 +183,7 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
 
 	/* see if we have an alias */
 	if (*path != '/') {
-		const char *q = memchr(path, '/', end - p);
+		const char *q = fdt_path_next_seperator(path, end - p);
 
 		if (!q)
 			q = end;
@@ -182,10 +201,10 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
 
 		while (*p == '/') {
 			p++;
-			if (p == end)
+			if (p == end || *p == ':')
 				return offset;
 		}
-		q = memchr(p, '/', end - p);
+		q = fdt_path_next_seperator(p, end - p);
 		if (! q)
 			q = end;
 
-- 
2.4.1

Re: [PATCH] libfdt: fdt_path_offset_*(): Fix handling of paths with options in them

[David Gibson]

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

On Thu, May 21, 2015 at 03:00:52PM +0200, Hans de Goede wrote:
> This is another approach at fixing the issues with paths with have options
> appended seperated by a ':' character.
> 
> commit b4150b59ae ("libfdt: Add fdt_path_offset_namelen()")
> 
> Is related to this, it allows the caller to specify to only look at part
> of the passed in path. But as experience with using this in the kernel has
> shown using this properly is quite hard since the options itself may have
> a '/' in them, also see the comment above the new fdt_path_next_seperator
> helper this commit adds.
> 
> So this commit, which currently is being used by u-boot, instead simply
> teaches fdt_path_offset() to just do the right thing when it encounters
> paths with a ':' in them.

I dislike this - it's building into the core path handling something
related to how external things happen to glue extra options on there.

I also don't see why it's necessary.  ':' shouldn't appear in paths,
so why can't you just strchr() for the first ':', pass the first path
to path_offset_namelen, and the last part to your option parsing code?
> 
> Cc: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  libfdt/fdt_ro.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index a65e4b5..9efbcb2 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -154,6 +154,25 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
>  	return fdt_subnode_offset_namelen(fdt, parentoffset, name, strlen(name));
>  }
>  
> +/*
> + * Find the next of path seperator, note we need to search for both '/' and ':'
> + * and then take the first one so that we do the rigth thing for e.g.
> + * "foo/bar:option" and "bar:option/otheroption", both of which happen, so
> + * first searching for either ':' or '/' does not work.
> + */
> +static const char *fdt_path_next_seperator(const char *path, int len)
> +{
> +	const char *sep1 = memchr(path, '/', len);
> +	const char *sep2 = memchr(path, ':', len);
> +
> +	if (sep1 && sep2)
> +		return (sep1 < sep2) ? sep1 : sep2;
> +	else if (sep1)
> +		return sep1;
> +	else
> +		return sep2;
> +}
> +
>  int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>  {
>  	const char *end = path + namelen;
> @@ -164,7 +183,7 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>  
>  	/* see if we have an alias */
>  	if (*path != '/') {
> -		const char *q = memchr(path, '/', end - p);
> +		const char *q = fdt_path_next_seperator(path, end - p);
>  
>  		if (!q)
>  			q = end;
> @@ -182,10 +201,10 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>  
>  		while (*p == '/') {
>  			p++;
> -			if (p == end)
> +			if (p == end || *p == ':')
>  				return offset;
>  		}
> -		q = memchr(p, '/', end - p);
> +		q = fdt_path_next_seperator(p, end - p);
>  		if (! q)
>  			q = end;
>  

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

Re: [PATCH] libfdt: fdt_path_offset_*(): Fix handling of paths with options in them

[Hans de Goede]

Hi,

On 29-05-15 14:26, David Gibson wrote:
> On Thu, May 21, 2015 at 03:00:52PM +0200, Hans de Goede wrote:
>> This is another approach at fixing the issues with paths with have options
>> appended seperated by a ':' character.
>>
>> commit b4150b59ae ("libfdt: Add fdt_path_offset_namelen()")
>>
>> Is related to this, it allows the caller to specify to only look at part
>> of the passed in path. But as experience with using this in the kernel has
>> shown using this properly is quite hard since the options itself may have
>> a '/' in them, also see the comment above the new fdt_path_next_seperator
>> helper this commit adds.
>>
>> So this commit, which currently is being used by u-boot, instead simply
>> teaches fdt_path_offset() to just do the right thing when it encounters
>> paths with a ':' in them.
>
> I dislike this - it's building into the core path handling something
> related to how external things happen to glue extra options on there.
>
> I also don't see why it's necessary.  ':' shouldn't appear in paths,
> so why can't you just strchr() for the first ':', pass the first path
> to path_offset_namelen, and the last part to your option parsing code?

When I wrote this (I was a bit slow in submitting it upstream)
path_offset_namelen did not exist yet.

But more importantly the Linux kernel is also doing the handling of ':'
at the same low level, see __of_find_node_by_path in drivers/of/base.c
in the kernel, which is the kernel equivalent of fdt_path_offset.

So to me it seems best to also handle this at the same level in libfdt
rather then expecting callers to deal with this, as that will lead to
more and more callers needing to be fixed when the same construct gets
used in more places.

Regards,

Hans

>>
>> Cc: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   libfdt/fdt_ro.c | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>> index a65e4b5..9efbcb2 100644
>> --- a/libfdt/fdt_ro.c
>> +++ b/libfdt/fdt_ro.c
>> @@ -154,6 +154,25 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
>>   	return fdt_subnode_offset_namelen(fdt, parentoffset, name, strlen(name));
>>   }
>>
>> +/*
>> + * Find the next of path seperator, note we need to search for both '/' and ':'
>> + * and then take the first one so that we do the rigth thing for e.g.
>> + * "foo/bar:option" and "bar:option/otheroption", both of which happen, so
>> + * first searching for either ':' or '/' does not work.
>> + */
>> +static const char *fdt_path_next_seperator(const char *path, int len)
>> +{
>> +	const char *sep1 = memchr(path, '/', len);
>> +	const char *sep2 = memchr(path, ':', len);
>> +
>> +	if (sep1 && sep2)
>> +		return (sep1 < sep2) ? sep1 : sep2;
>> +	else if (sep1)
>> +		return sep1;
>> +	else
>> +		return sep2;
>> +}
>> +
>>   int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>>   {
>>   	const char *end = path + namelen;
>> @@ -164,7 +183,7 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>>
>>   	/* see if we have an alias */
>>   	if (*path != '/') {
>> -		const char *q = memchr(path, '/', end - p);
>> +		const char *q = fdt_path_next_seperator(path, end - p);
>>
>>   		if (!q)
>>   			q = end;
>> @@ -182,10 +201,10 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>>
>>   		while (*p == '/') {
>>   			p++;
>> -			if (p == end)
>> +			if (p == end || *p == ':')
>>   				return offset;
>>   		}
>> -		q = memchr(p, '/', end - p);
>> +		q = fdt_path_next_seperator(p, end - p);
>>   		if (! q)
>>   			q = end;
>>
>

Re: [PATCH] libfdt: fdt_path_offset_*(): Fix handling of paths with options in them

[Peter Hurley]

On 05/30/2015 03:52 AM, Hans de Goede wrote:
> Hi,
> 
> On 29-05-15 14:26, David Gibson wrote:
>> On Thu, May 21, 2015 at 03:00:52PM +0200, Hans de Goede wrote:
>>> This is another approach at fixing the issues with paths with have options
>>> appended seperated by a ':' character.
>>>
>>> commit b4150b59ae ("libfdt: Add fdt_path_offset_namelen()")
>>>
>>> Is related to this, it allows the caller to specify to only look at part
>>> of the passed in path. But as experience with using this in the kernel has
>>> shown using this properly is quite hard since the options itself may have
>>> a '/' in them, also see the comment above the new fdt_path_next_seperator
>>> helper this commit adds.
>>>
>>> So this commit, which currently is being used by u-boot, instead simply
>>> teaches fdt_path_offset() to just do the right thing when it encounters
>>> paths with a ':' in them.
>>
>> I dislike this - it's building into the core path handling something
>> related to how external things happen to glue extra options on there.
>>
>> I also don't see why it's necessary.  ':' shouldn't appear in paths,
>> so why can't you just strchr() for the first ':', pass the first path
>> to path_offset_namelen, and the last part to your option parsing code?
> 
> When I wrote this (I was a bit slow in submitting it upstream)
> path_offset_namelen did not exist yet.
> 
> But more importantly the Linux kernel is also doing the handling of ':'
> at the same low level, see __of_find_node_by_path in drivers/of/base.c
> in the kernel, which is the kernel equivalent of fdt_path_offset.

Not quite; that code is for _after_ the devicetree is unflattened.

The patch-on-deck for handling stdout-path options in earlycon code,
[v3] of:earlycon: Fix 'stdout-path' with ':' path terminator,
does just as David suggests.

Regards,
Peter Hurley

> So to me it seems best to also handle this at the same level in libfdt
> rather then expecting callers to deal with this, as that will lead to
> more and more callers needing to be fixed when the same construct gets
> used in more places.
> 
> Regards,
> 
> Hans
> 
>>>
>>> Cc: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>   libfdt/fdt_ro.c | 25 ++++++++++++++++++++++---
>>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>>> index a65e4b5..9efbcb2 100644
>>> --- a/libfdt/fdt_ro.c
>>> +++ b/libfdt/fdt_ro.c
>>> @@ -154,6 +154,25 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
>>>       return fdt_subnode_offset_namelen(fdt, parentoffset, name, strlen(name));
>>>   }
>>>
>>> +/*
>>> + * Find the next of path seperator, note we need to search for both '/' and ':'
>>> + * and then take the first one so that we do the rigth thing for e.g.
>>> + * "foo/bar:option" and "bar:option/otheroption", both of which happen, so
>>> + * first searching for either ':' or '/' does not work.
>>> + */
>>> +static const char *fdt_path_next_seperator(const char *path, int len)
>>> +{
>>> +    const char *sep1 = memchr(path, '/', len);
>>> +    const char *sep2 = memchr(path, ':', len);
>>> +
>>> +    if (sep1 && sep2)
>>> +        return (sep1 < sep2) ? sep1 : sep2;
>>> +    else if (sep1)
>>> +        return sep1;
>>> +    else
>>> +        return sep2;
>>> +}
>>> +
>>>   int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>>>   {
>>>       const char *end = path + namelen;
>>> @@ -164,7 +183,7 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>>>
>>>       /* see if we have an alias */
>>>       if (*path != '/') {
>>> -        const char *q = memchr(path, '/', end - p);
>>> +        const char *q = fdt_path_next_seperator(path, end - p);
>>>
>>>           if (!q)
>>>               q = end;
>>> @@ -182,10 +201,10 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>>>
>>>           while (*p == '/') {
>>>               p++;
>>> -            if (p == end)
>>> +            if (p == end || *p == ':')
>>>                   return offset;
>>>           }
>>> -        q = memchr(p, '/', end - p);
>>> +        q = fdt_path_next_seperator(p, end - p);
>>>           if (! q)
>>>               q = end;
>>>

Re: [PATCH] libfdt: fdt_path_offset_*(): Fix handling of paths with options in them

[David Gibson]

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

On Sat, May 30, 2015 at 09:52:23AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 29-05-15 14:26, David Gibson wrote:
> >On Thu, May 21, 2015 at 03:00:52PM +0200, Hans de Goede wrote:
> >>This is another approach at fixing the issues with paths with have options
> >>appended seperated by a ':' character.
> >>
> >>commit b4150b59ae ("libfdt: Add fdt_path_offset_namelen()")
> >>
> >>Is related to this, it allows the caller to specify to only look at part
> >>of the passed in path. But as experience with using this in the kernel has
> >>shown using this properly is quite hard since the options itself may have
> >>a '/' in them, also see the comment above the new fdt_path_next_seperator
> >>helper this commit adds.
> >>
> >>So this commit, which currently is being used by u-boot, instead simply
> >>teaches fdt_path_offset() to just do the right thing when it encounters
> >>paths with a ':' in them.
> >
> >I dislike this - it's building into the core path handling something
> >related to how external things happen to glue extra options on there.
> >
> >I also don't see why it's necessary.  ':' shouldn't appear in paths,
> >so why can't you just strchr() for the first ':', pass the first path
> >to path_offset_namelen, and the last part to your option parsing code?
> 
> When I wrote this (I was a bit slow in submitting it upstream)
> path_offset_namelen did not exist yet.
> 
> But more importantly the Linux kernel is also doing the handling of ':'
> at the same low level, see __of_find_node_by_path in drivers/of/base.c
> in the kernel, which is the kernel equivalent of fdt_path_offset.
> 
> So to me it seems best to also handle this at the same level in libfdt
> rather then expecting callers to deal with this, as that will lead to
> more and more callers needing to be fixed when the same construct gets
> used in more places.

No, sorry.  It might be used in several places, but it's still a long
way from being universal enough that I'm willing to fold it into the
core handling.

A helper function to handle this case, I'd be happy to apply.  Folding
it into the core semantics, no.

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