[PATCH] dtc: fix for_each_*() to skip first object if deleted

[PATCH] dtc: fix for_each_*() to skip first object if deleted

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The previous definition of e.g. for_each_*() would always include the
very first object within the list of processed labels, irrespective of
whether it was marked deleted, since the deleted flag was not checked
on the first object, but only on any "next" object.

Fix for_each_*() to call skip_deleted_*() prior to every loop iteration,
including the first, i.e. as part of the for loop test expression rather
than as part of the increment statement, to correct this.

Incidentally, this change is why commit 45013d8 dtc: "Add ability to
delete nodes and properties" only caused two "make checkm" failures;
only two tests actually use multiple labels on the same property or
node. With this current change applied, but commit 317a5d9 "dtc: zero
out new label objects" reverted, "make checkm" fails 29 times; i.e.
for every test that uses any labels at all.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Rob, Grant, now that you're listed as maintainers for the Linux kernel's
copy of dtc, will you automatically apply patches there as they are
accepted into upstream dtc, or should I still create "backport" patches
and send them to you separately?

 dtc.h |   23 +++++++++++------------
 1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/dtc.h b/dtc.h
index d501c86..88da264 100644
--- a/dtc.h
+++ b/dtc.h
@@ -161,47 +161,46 @@ struct node {
 	struct label *labels;
 };
 
-static inline struct label *for_each_label_next(struct label *l)
+static inline struct label *skip_deleted_labels(struct label *l)
 {
-	do {
+	while (l && l->deleted)
 		l = l->next;
-	} while (l && l->deleted);
 
 	return l;
 }
 
 #define for_each_label(l0, l) \
-	for ((l) = (l0); (l); (l) = for_each_label_next(l))
+	for ((l) = (l0); ((l) = skip_deleted_labels(l)); (l) = (l)->next)
 
 #define for_each_label_withdel(l0, l) \
 	for ((l) = (l0); (l); (l) = (l)->next)
 
-static inline struct property *for_each_property_next(struct property *p)
+static inline struct property *skip_deleted_properties(struct property *p)
 {
-	do {
+	while (p && p->deleted)
 		p = p->next;
-	} while (p && p->deleted);
 
 	return p;
 }
 
 #define for_each_property(n, p) \
-	for ((p) = (n)->proplist; (p); (p) = for_each_property_next(p))
+	for ((p) = (n)->proplist; ((p) = skip_deleted_properties(p)); \
+		(p) = (p)->next)
 
 #define for_each_property_withdel(n, p) \
 	for ((p) = (n)->proplist; (p); (p) = (p)->next)
 
-static inline struct node *for_each_child_next(struct node *c)
+static inline struct node *skip_deleted_children(struct node *c)
 {
-	do {
+	while (c && c->deleted)
 		c = c->next_sibling;
-	} while (c && c->deleted);
 
 	return c;
 }
 
 #define for_each_child(n, c) \
-	for ((c) = (n)->children; (c); (c) = for_each_child_next(c))
+	for ((c) = (n)->children; ((c) = skip_deleted_children(c)); \
+		(c) = (c)->next_sibling)
 
 #define for_each_child_withdel(n, c) \
 	for ((c) = (n)->children; (c); (c) = (c)->next_sibling)
-- 
1.7.0.4

Re: [PATCH] dtc: fix for_each_*() to skip first object if deleted

[Rob Herring]

On 10/03/2012 05:32 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The previous definition of e.g. for_each_*() would always include the
> very first object within the list of processed labels, irrespective of
> whether it was marked deleted, since the deleted flag was not checked
> on the first object, but only on any "next" object.
> 
> Fix for_each_*() to call skip_deleted_*() prior to every loop iteration,
> including the first, i.e. as part of the for loop test expression rather
> than as part of the increment statement, to correct this.
> 
> Incidentally, this change is why commit 45013d8 dtc: "Add ability to
> delete nodes and properties" only caused two "make checkm" failures;
> only two tests actually use multiple labels on the same property or
> node. With this current change applied, but commit 317a5d9 "dtc: zero
> out new label objects" reverted, "make checkm" fails 29 times; i.e.
> for every test that uses any labels at all.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Rob, Grant, now that you're listed as maintainers for the Linux kernel's
> copy of dtc, will you automatically apply patches there as they are
> accepted into upstream dtc, or should I still create "backport" patches
> and send them to you separately?

I guess the fundamental question is when do we sync upstream dtc: in
sync with the kernel cycles or based on some upstream dtc milestone?
There's not really releases of dtc unless you go ask for one. So I don't
think any particular commit is more or less stable. I would guess most
users are using the in kernel version, so upstream is probably not
getting tested enough that we would gain some test coverage by waiting.
This was my rational for picking up the changes for 3.7.

Assuming we go with pick-up all the dtc changes every kernel cycle, it
should be simple enough to script picking up commits from upstream. I'm
assuming it is just a path fix-up? In that case, it wouldn't be
necessary to post both versions of patches. Now, if someone wants to do
that script and just provide me a git branch to pull, I would be more
than happy to take it.

Rob

> 
>  dtc.h |   23 +++++++++++------------
>  1 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/dtc.h b/dtc.h
> index d501c86..88da264 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -161,47 +161,46 @@ struct node {
>  	struct label *labels;
>  };
>  
> -static inline struct label *for_each_label_next(struct label *l)
> +static inline struct label *skip_deleted_labels(struct label *l)
>  {
> -	do {
> +	while (l && l->deleted)
>  		l = l->next;
> -	} while (l && l->deleted);
>  
>  	return l;
>  }
>  
>  #define for_each_label(l0, l) \
> -	for ((l) = (l0); (l); (l) = for_each_label_next(l))
> +	for ((l) = (l0); ((l) = skip_deleted_labels(l)); (l) = (l)->next)
>  
>  #define for_each_label_withdel(l0, l) \
>  	for ((l) = (l0); (l); (l) = (l)->next)
>  
> -static inline struct property *for_each_property_next(struct property *p)
> +static inline struct property *skip_deleted_properties(struct property *p)
>  {
> -	do {
> +	while (p && p->deleted)
>  		p = p->next;
> -	} while (p && p->deleted);
>  
>  	return p;
>  }
>  
>  #define for_each_property(n, p) \
> -	for ((p) = (n)->proplist; (p); (p) = for_each_property_next(p))
> +	for ((p) = (n)->proplist; ((p) = skip_deleted_properties(p)); \
> +		(p) = (p)->next)
>  
>  #define for_each_property_withdel(n, p) \
>  	for ((p) = (n)->proplist; (p); (p) = (p)->next)
>  
> -static inline struct node *for_each_child_next(struct node *c)
> +static inline struct node *skip_deleted_children(struct node *c)
>  {
> -	do {
> +	while (c && c->deleted)
>  		c = c->next_sibling;
> -	} while (c && c->deleted);
>  
>  	return c;
>  }
>  
>  #define for_each_child(n, c) \
> -	for ((c) = (n)->children; (c); (c) = for_each_child_next(c))
> +	for ((c) = (n)->children; ((c) = skip_deleted_children(c)); \
> +		(c) = (c)->next_sibling)
>  
>  #define for_each_child_withdel(n, c) \
>  	for ((c) = (n)->children; (c); (c) = (c)->next_sibling)
> 

Re: [PATCH] dtc: fix for_each_*() to skip first object if deleted

[David Gibson]

On Wed, Oct 03, 2012 at 04:32:54PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The previous definition of e.g. for_each_*() would always include the
> very first object within the list of processed labels, irrespective of
> whether it was marked deleted, since the deleted flag was not checked
> on the first object, but only on any "next" object.
> 
> Fix for_each_*() to call skip_deleted_*() prior to every loop iteration,
> including the first, i.e. as part of the for loop test expression rather
> than as part of the increment statement, to correct this.
> 
> Incidentally, this change is why commit 45013d8 dtc: "Add ability to
> delete nodes and properties" only caused two "make checkm" failures;
> only two tests actually use multiple labels on the same property or
> node. With this current change applied, but commit 317a5d9 "dtc: zero
> out new label objects" reverted, "make checkm" fails 29 times; i.e.
> for every test that uses any labels at all.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Oops, we definitely want this fix, or one like it.  This one really
could do with a testcase - or specifically extending your existing
deletion testcases to cover this variant.


> diff --git a/dtc.h b/dtc.h
> index d501c86..88da264 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -161,47 +161,46 @@ struct node {
>  	struct label *labels;
>  };
>  
> -static inline struct label *for_each_label_next(struct label *l)
> +static inline struct label *skip_deleted_labels(struct label *l)
>  {
> -	do {
> +	while (l && l->deleted)
>  		l = l->next;
> -	} while (l && l->deleted);
>  
>  	return l;
>  }
>  
>  #define for_each_label(l0, l) \
> -	for ((l) = (l0); (l); (l) = for_each_label_next(l))
> +	for ((l) = (l0); ((l) = skip_deleted_labels(l)); (l) = (l)->next)

I think there's an easier way to implement this though, rather than
having these skip functions, it should work to just do:

#define for_each_label(l0, l) \
	for_each_label_witdel(l0, l) \
		if (!(l)->deleted)

And likewise for the other variants.

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

Re: [PATCH] dtc: fix for_each_*() to skip first object if deleted

[Stephen Warren]

On 10/03/2012 05:42 PM, Rob Herring wrote:
> On 10/03/2012 05:32 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> The previous definition of e.g. for_each_*() would always include the
>> very first object within the list of processed labels, irrespective of
>> whether it was marked deleted, since the deleted flag was not checked
>> on the first object, but only on any "next" object.
>>
>> Fix for_each_*() to call skip_deleted_*() prior to every loop iteration,
>> including the first, i.e. as part of the for loop test expression rather
>> than as part of the increment statement, to correct this.
>>
>> Incidentally, this change is why commit 45013d8 dtc: "Add ability to
>> delete nodes and properties" only caused two "make checkm" failures;
>> only two tests actually use multiple labels on the same property or
>> node. With this current change applied, but commit 317a5d9 "dtc: zero
>> out new label objects" reverted, "make checkm" fails 29 times; i.e.
>> for every test that uses any labels at all.
>>
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> Rob, Grant, now that you're listed as maintainers for the Linux kernel's
>> copy of dtc, will you automatically apply patches there as they are
>> accepted into upstream dtc, or should I still create "backport" patches
>> and send them to you separately?
> 
> I guess the fundamental question is when do we sync upstream dtc:
...
> Assuming we go with pick-up all the dtc changes every kernel cycle, it
> should be simple enough to script picking up commits from upstream. I'm
> assuming it is just a path fix-up? In that case, it wouldn't be
> necessary to post both versions of patches. Now, if someone wants to do
> that script and just provide me a git branch to pull, I would be more
> than happy to take it.

There are a couple of things that make scripting this slightly non-trivial:

a) The kernel has its own Makefile, so when Makefile.dtc is changed, the
kernel's Makefile needs equivalent changes.

I guess a script could just check for Makefile edits, and flag the
commit as needing manual fixup later or something.

b) The kernel doesn't run flex/bison, but rather relies on having
checked-in copies of their output with "_shipped" names.

I guess this wouldn't be too hard to automate.

c) And of course, some files don't get checked into the kernel (e.g.
tests/), but that and the path change should be easy to handle.

[PATCH] dtc: fix for_each_*() to skip first object if deleted

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The previous definition of for_each_*() would always include the very
first object within the list, irrespective of whether it was marked
deleted, since the deleted flag was not checked on the first object,
but only on any "next" object.

Fix for_each_*() to check the deleted flag in the loop body every
iteration to correct this.

(upstream dtc commit 1762ab42ef77db7ab2776d0d6cba3515150f518a)

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 scripts/dtc/dtc.h |   44 ++++++++++----------------------------------
 1 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/scripts/dtc/dtc.h b/scripts/dtc/dtc.h
index d501c86..3e42a07 100644
--- a/scripts/dtc/dtc.h
+++ b/scripts/dtc/dtc.h
@@ -161,51 +161,27 @@ struct node {
 	struct label *labels;
 };
 
-static inline struct label *for_each_label_next(struct label *l)
-{
-	do {
-		l = l->next;
-	} while (l && l->deleted);
-
-	return l;
-}
-
-#define for_each_label(l0, l) \
-	for ((l) = (l0); (l); (l) = for_each_label_next(l))
-
 #define for_each_label_withdel(l0, l) \
 	for ((l) = (l0); (l); (l) = (l)->next)
 
-static inline struct property *for_each_property_next(struct property *p)
-{
-	do {
-		p = p->next;
-	} while (p && p->deleted);
-
-	return p;
-}
-
-#define for_each_property(n, p) \
-	for ((p) = (n)->proplist; (p); (p) = for_each_property_next(p))
+#define for_each_label(l0, l) \
+	for_each_label_withdel(l0, l) \
+		if (!(l)->deleted)
 
 #define for_each_property_withdel(n, p) \
 	for ((p) = (n)->proplist; (p); (p) = (p)->next)
 
-static inline struct node *for_each_child_next(struct node *c)
-{
-	do {
-		c = c->next_sibling;
-	} while (c && c->deleted);
-
-	return c;
-}
-
-#define for_each_child(n, c) \
-	for ((c) = (n)->children; (c); (c) = for_each_child_next(c))
+#define for_each_property(n, p) \
+	for_each_property_withdel(n, p) \
+		if (!(p)->deleted)
 
 #define for_each_child_withdel(n, c) \
 	for ((c) = (n)->children; (c); (c) = (c)->next_sibling)
 
+#define for_each_child(n, c) \
+	for_each_child_withdel(n, c) \
+		if (!(c)->deleted)
+
 void add_label(struct label **labels, char *label);
 void delete_labels(struct label **labels);
 
-- 
1.7.0.4

Re: [PATCH] dtc: fix for_each_*() to skip first object if deleted

[Rob Herring]

On 10/08/2012 05:15 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The previous definition of for_each_*() would always include the very
> first object within the list, irrespective of whether it was marked
> deleted, since the deleted flag was not checked on the first object,
> but only on any "next" object.
> 
> Fix for_each_*() to check the deleted flag in the loop body every
> iteration to correct this.
> 
> (upstream dtc commit 1762ab42ef77db7ab2776d0d6cba3515150f518a)
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Applied.

Thanks for sending this before I got around to asking you to since a
script to do this is not yet in place. :)

Rob

> ---
>  scripts/dtc/dtc.h |   44 ++++++++++----------------------------------
>  1 files changed, 10 insertions(+), 34 deletions(-)
> 
> diff --git a/scripts/dtc/dtc.h b/scripts/dtc/dtc.h
> index d501c86..3e42a07 100644
> --- a/scripts/dtc/dtc.h
> +++ b/scripts/dtc/dtc.h
> @@ -161,51 +161,27 @@ struct node {
>  	struct label *labels;
>  };
>  
> -static inline struct label *for_each_label_next(struct label *l)
> -{
> -	do {
> -		l = l->next;
> -	} while (l && l->deleted);
> -
> -	return l;
> -}
> -
> -#define for_each_label(l0, l) \
> -	for ((l) = (l0); (l); (l) = for_each_label_next(l))
> -
>  #define for_each_label_withdel(l0, l) \
>  	for ((l) = (l0); (l); (l) = (l)->next)
>  
> -static inline struct property *for_each_property_next(struct property *p)
> -{
> -	do {
> -		p = p->next;
> -	} while (p && p->deleted);
> -
> -	return p;
> -}
> -
> -#define for_each_property(n, p) \
> -	for ((p) = (n)->proplist; (p); (p) = for_each_property_next(p))
> +#define for_each_label(l0, l) \
> +	for_each_label_withdel(l0, l) \
> +		if (!(l)->deleted)
>  
>  #define for_each_property_withdel(n, p) \
>  	for ((p) = (n)->proplist; (p); (p) = (p)->next)
>  
> -static inline struct node *for_each_child_next(struct node *c)
> -{
> -	do {
> -		c = c->next_sibling;
> -	} while (c && c->deleted);
> -
> -	return c;
> -}
> -
> -#define for_each_child(n, c) \
> -	for ((c) = (n)->children; (c); (c) = for_each_child_next(c))
> +#define for_each_property(n, p) \
> +	for_each_property_withdel(n, p) \
> +		if (!(p)->deleted)
>  
>  #define for_each_child_withdel(n, c) \
>  	for ((c) = (n)->children; (c); (c) = (c)->next_sibling)
>  
> +#define for_each_child(n, c) \
> +	for_each_child_withdel(n, c) \
> +		if (!(c)->deleted)
> +
>  void add_label(struct label **labels, char *label);
>  void delete_labels(struct label **labels);
>  
>