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

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

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