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

Message ID 20141128163807.GN2361@bivouac.eciton.net
State New
Headers show

Commit Message

Leif Lindholm Nov. 28, 2014, 4:38 p.m.
On Fri, Nov 28, 2014 at 03:25:12PM +0000, Grant Likely wrote:
> On Fri, 28 Nov 2014 11:34:28 +0000
> , Leif Lindholm <leif.lindholm@linaro.org>
>  wrote:
> > On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> > > > +	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?
> 
> Perfect, applied with one fixup below...

Thanks!

And since it's Friday... *cough*

From 5c469dad81961164c444da7d6c4e1c5b1c097aab Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Fri, 28 Nov 2014 16:27:25 +0000
Subject: [PATCH] of: fix options clearing when path is "/"

The addition of the optional options extraction on
of_find_node_opts_by_path failed to clear incoming options pointer
if the specified path was "/".

Resolve this case, and add a test to detect it.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 drivers/of/base.c     |   11 ++++++-----
 drivers/of/selftest.c |    5 +++++
 2 files changed, 11 insertions(+), 5 deletions(-)

Comments

Grant Likely Nov. 28, 2014, 11:57 p.m. | #1
On Fri, Nov 28, 2014 at 4:38 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Nov 28, 2014 at 03:25:12PM +0000, Grant Likely wrote:
>> On Fri, 28 Nov 2014 11:34:28 +0000
>> , Leif Lindholm <leif.lindholm@linaro.org>
>>  wrote:
>> > On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
>> > > > +       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?
>>
>> Perfect, applied with one fixup below...
>
> Thanks!

Fixups merged. Thanks.

g.

>
> And since it's Friday... *cough*
>
> From 5c469dad81961164c444da7d6c4e1c5b1c097aab Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Fri, 28 Nov 2014 16:27:25 +0000
> Subject: [PATCH] of: fix options clearing when path is "/"
>
> The addition of the optional options extraction on
> of_find_node_opts_by_path failed to clear incoming options pointer
> if the specified path was "/".
>
> Resolve this case, and add a test to detect it.
>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  drivers/of/base.c     |   11 ++++++-----
>  drivers/of/selftest.c |    5 +++++
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5e16c6b..32664d1 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -743,15 +743,16 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>         struct device_node *np = NULL;
>         struct property *pp;
>         unsigned long flags;
> -       char *separator;
> +       char *separator = NULL;
> +
> +       if (opts) {
> +               separator = strchr(path, ':');
> +               *opts = separator ? separator + 1 : NULL;
> +       }
>
>         if (strcmp(path, "/") == 0)
>                 return of_node_get(of_allnodes);
>
> -       separator = strchr(path, ':');
> -       if (opts)
> -               *opts = separator ? separator + 1 : NULL;
> -
>         /* The path could begin with an alias */
>         if (*path != '/') {
>                 char *p = strchrnul(path, '/');
> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
> index 721b2a4..914f0ae 100644
> --- a/drivers/of/selftest.c
> +++ b/drivers/of/selftest.c
> @@ -100,6 +100,11 @@ static void __init of_selftest_find_node_by_name(void)
>         np = of_find_node_opts_by_path("testcase-alias", &options);
>         selftest(np && !options, "option clearing test failed\n");
>         of_node_put(np);
> +
> +       options = "testoption";
> +       np = of_find_node_opts_by_path("/", &options);
> +       selftest(np && !options, "option clearing root node test failed\n");
> +       of_node_put(np);
>  }
>
>  static void __init of_selftest_dynamic(void)
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5e16c6b..32664d1 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -743,15 +743,16 @@  struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 	struct device_node *np = NULL;
 	struct property *pp;
 	unsigned long flags;
-	char *separator;
+	char *separator = NULL;
+
+	if (opts) {
+		separator = strchr(path, ':');
+		*opts = separator ? separator + 1 : NULL;
+	}
 
 	if (strcmp(path, "/") == 0)
 		return of_node_get(of_allnodes);
 
-	separator = strchr(path, ':');
-	if (opts)
-		*opts = separator ? separator + 1 : NULL;
-
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		char *p = strchrnul(path, '/');
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index 721b2a4..914f0ae 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -100,6 +100,11 @@  static void __init of_selftest_find_node_by_name(void)
 	np = of_find_node_opts_by_path("testcase-alias", &options);
 	selftest(np && !options, "option clearing test failed\n");
 	of_node_put(np);
+
+	options = "testoption";
+	np = of_find_node_opts_by_path("/", &options);
+	selftest(np && !options, "option clearing root node test failed\n");
+	of_node_put(np);
 }
 
 static void __init of_selftest_dynamic(void)