Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Leif Lindholm]

On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> On Thu, 27 Nov 2014 17:56:06 +0000
> , Leif Lindholm <leif.lindholm@xxxxxxxxxx>
>  wrote:
> > Update of_find_node_by_path():
> > 1) Rename function to of_find_node_opts_by_path(), adding an optional
> >    pointer argument. Provide a static inline wrapper version of
> >    of_find_node_by_path() which calls the new function with NULL as
> >    the optional argument.
> > 2) Ignore any part of the path beyond and including the ':' separator.
> > 3) Set the new provided pointer argument to the beginning of the string
> >    following the ':' separator.
> > 4: Add tests.
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@xxxxxxxxxx>
> > ---
> >  drivers/of/base.c     |   21 +++++++++++++++++----
> >  drivers/of/selftest.c |   12 ++++++++++++
> >  include/linux/of.h    |   14 +++++++++++++-
> >  3 files changed, 42 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 3823edf..7f0e5f7 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >  {
> >  	struct device_node *child;
> >  	int len = strchrnul(path, '/') - path;
> > +	int term;
> >  
> >  	if (!len)
> >  		return NULL;
> >  
> > +	term = strchrnul(path, ':') - path;
> > +	if (term < len)
> > +		len = term;
> > +
> >  	__for_each_child_of_node(parent, child) {
> >  		const char *name = strrchr(child->full_name, '/');
> >  		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> > @@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >  }
> >  
> >  /**
> > - *	of_find_node_by_path - Find a node matching a full OF path
> > + *	of_find_node_opts_by_path - Find a node matching a full OF path
> >   *	@path: Either the full path to match, or if the path does not
> >   *	       start with '/', the name of a property of the /aliases
> >   *	       node (an alias).  In the case of an alias, the node
> >   *	       matching the alias' value will be returned.
> > + *	@opts: Address of a pointer into which to store the start of
> > + *	       an options string appended to the end of the path with
> > + *	       a ':' separator.
> >   *
> >   *	Valid paths:
> >   *		/foo/bar	Full path
> > @@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >   *	Returns a node pointer with refcount incremented, use
> >   *	of_node_put() on it when done.
> >   */
> > -struct device_node *of_find_node_by_path(const char *path)
> > +struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
> >  {
> >  	struct device_node *np = NULL;
> >  	struct property *pp;
> >  	unsigned long flags;
> > +	char *separator;
> >  
> >  	if (strcmp(path, "/") == 0)
> >  		return of_node_get(of_allnodes);
> >  
> > +	separator = strchr(path, ':');
> > +	if (separator && opts)
> > +		*opts = separator + 1;
> > +
> 
> What about when there are no opts? Do we require the caller to make sure
> opts is NULL before calling the function (which sounds like a good
> source of bugs) or do we clear it on successful return?
> 
> I think if opts is passed in, but there are no options, then it should
> set *opts = NULL.

Yeah, oops.

> There should be test cases for this also. Must set opts to NULL on
> successful return, and (I think) should leave opts alone on an
> unsuccessful search.

I would actually argue for always nuking the opts - since that could
(theoretically) prevent something working by accident in spite of
error conditions.

How about the below?

/
    Leif