Re: [PATCH 06/10] tracing/filter: Change count_leafs function to use walk_pred_tree

[Steven Rostedt <rostedt@goodmis.org>]

I really like these two patches (5 and 6) as I hated the duplicate code
of the two tree walks. But... see below.


On Thu, 2011-08-04 at 12:08 +0200, Jiri Olsa wrote:
> Changing count_leafs function to use unified predicates tree
> processing.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  kernel/trace/trace_events_filter.c |   45 +++++++++--------------------------
>  1 files changed, 12 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 5b889d4..14a9dad 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -1418,43 +1418,22 @@ static int check_pred_tree(struct event_filter *filter,
>  			      check_pred_tree_cb, &data);
>  }
>  
> -static int count_leafs(struct filter_pred *preds, struct filter_pred *root)
> +static int count_leafs_cb(enum move_type move, struct filter_pred *pred,
> +			  int *err, void *data)
>  {
> -	struct filter_pred *pred;
> -	enum move_type move = MOVE_DOWN;
> -	int count = 0;
> -	int done = 0;
> +	int *count = data;
>  
> -	pred = root;
> +	if ((move == MOVE_DOWN) &&
> +	    (pred->left == FILTER_PRED_INVALID))
> +		(*count)++;
>  
> -	do {
> -		switch (move) {
> -		case MOVE_DOWN:
> -			if (pred->left != FILTER_PRED_INVALID) {
> -				pred = &preds[pred->left];
> -				continue;
> -			}
> -			/* A leaf at the root is just a leaf in the tree */
> -			if (pred == root)
> -				return 1;
> -			count++;
> -			pred = get_pred_parent(pred, preds,
> -					       pred->parent, &move);
> -			continue;
> -		case MOVE_UP_FROM_LEFT:
> -			pred = &preds[pred->right];
> -			move = MOVE_DOWN;
> -			continue;
> -		case MOVE_UP_FROM_RIGHT:
> -			if (pred == root)
> -				break;
> -			pred = get_pred_parent(pred, preds,
> -					       pred->parent, &move);
> -			continue;
> -		}
> -		done = 1;
> -	} while (!done);
> +	return WALK_PRED_DEFAULT;
> +}
>  
> +static int count_leafs(struct filter_pred *preds, struct filter_pred *root)
> +{
> +	int count = 0;
> +	WARN_ON(walk_pred_tree(preds, root, count_leafs_cb, &count));

Please do not put functionality in a WARN_ON(). Some people have
WARN_ON() turn into nops.

Make this a:

	ret = walk_pred_tree(preds, root, count_leafs_cb, &count);
	WARN_ON(ret);

-- Steve

>  	return count;
>  }
>