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

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

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