[PATCH 0/3] tracing: Add event filter logic for !(...)

[PATCH 0/3] tracing: Add event filter logic for !(...)

[Steven Rostedt]

There's no changes to the first patch that you can see from here:

  http://lkml.kernel.org/r/20141202153548.1ce7177e@gandalf.local.home

But I added two more patches:

 1) adds the logic to do ! with the AND and OR cases. Like:

  !(prev_pid == 0 && next_pid == 1)

 2) I added ftracetests selftest to test a bunch of filter cases.
    I can probably add more, but this is a start.

Steven Rostedt (Red Hat) (3):
      tracing: Add NOT to filtering logic
      tracing: Allow NOT to filter AND and OR clauses
      ftracetests: Add test to test event filter logic

----
 kernel/trace/trace_events_filter.c                 |  29 ++-
 .../selftests/ftrace/test.d/ftrace/filter.tc       | 200 +++++++++++++++++++++
 2 files changed, 223 insertions(+), 6 deletions(-)

[PATCH 1/3] tracing: Add NOT to filtering logic

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-Add-NOT-to-filtering-logic.patch --]
[-- Type: text/plain, Size: 3027 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Ted noticed that he could not filter on an event for a bit being cleared.
That's because the filtering logic only tests event fields with a limited
number of comparisons which, for bit logic, only include "&", which can
test if a bit is set, but there's no good way to see if a bit is clear.

This adds a way to do: !(field & 2048)

Which returns true if the bit is not set, and false otherwise.

Note, currently !(field1 == 10 && field2 == 15) is not supported.
That is, the 'not' only works for direct comparisons, not for the
AND and OR logic.

Link: http://lkml.kernel.org/r/20141202021912.GA29096@thunk.org
Link: http://lkml.kernel.org/r/20141202120430.71979060@gandalf.local.home

Suggested-by: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_filter.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 7a8c1528e141..e6a33db83856 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -45,6 +45,7 @@ enum filter_op_ids
 	OP_GT,
 	OP_GE,
 	OP_BAND,
+	OP_NOT,
 	OP_NONE,
 	OP_OPEN_PAREN,
 };
@@ -67,6 +68,7 @@ static struct filter_op filter_ops[] = {
 	{ OP_GT,	">",		5 },
 	{ OP_GE,	">=",		5 },
 	{ OP_BAND,	"&",		6 },
+	{ OP_NOT,	"!",		6 },
 	{ OP_NONE,	"OP_NONE",	0 },
 	{ OP_OPEN_PAREN, "(",		0 },
 };
@@ -85,6 +87,7 @@ enum {
 	FILT_ERR_MISSING_FIELD,
 	FILT_ERR_INVALID_FILTER,
 	FILT_ERR_IP_FIELD_ONLY,
+	FILT_ERR_ILLEGAL_NOT_OP,
 };
 
 static char *err_text[] = {
@@ -101,6 +104,7 @@ static char *err_text[] = {
 	"Missing field name and/or value",
 	"Meaningless filter expression",
 	"Only 'ip' field is supported for function trace",
+	"Illegal use of '!'",
 };
 
 struct opstack_op {
@@ -139,6 +143,7 @@ struct pred_stack {
 	int			index;
 };
 
+/* If not of not match is equal to not of not, then it is a match */
 #define DEFINE_COMPARISON_PRED(type)					\
 static int filter_pred_##type(struct filter_pred *pred, void *event)	\
 {									\
@@ -166,7 +171,7 @@ static int filter_pred_##type(struct filter_pred *pred, void *event)	\
 		break;							\
 	}								\
 									\
-	return match;							\
+	return !!match == !pred->not;					\
 }
 
 #define DEFINE_EQUALITY_PRED(size)					\
@@ -1028,7 +1033,7 @@ static int init_pred(struct filter_parse_state *ps,
 	}
 
 	if (pred->op == OP_NE)
-		pred->not = 1;
+		pred->not ^= 1;
 
 	pred->fn = fn;
 	return 0;
@@ -1590,6 +1595,17 @@ static int replace_preds(struct ftrace_event_call *call,
 			continue;
 		}
 
+		if (elt->op == OP_NOT) {
+			if (!n_preds || operand1 || operand2) {
+				parse_error(ps, FILT_ERR_ILLEGAL_NOT_OP, 0);
+				err = -EINVAL;
+				goto fail;
+			}
+			if (!dry_run)
+				filter->preds[n_preds - 1].not ^= 1;
+			continue;
+		}
+
 		if (WARN_ON(n_preds++ == MAX_FILTER_PRED)) {
 			parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0);
 			err = -ENOSPC;
-- 
2.1.3

[PATCH 2/3] tracing: Allow NOT to filter AND and OR clauses

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-Allow-NOT-to-filter-AND-and-OR-clauses.patch --]
[-- Type: text/plain, Size: 1422 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Add support to allow not "!" for and (&&) and (||). That is:

 !(field1 == X && field2 == Y)

Where the value of the full clause will be notted.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_filter.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index e6a33db83856..ced69da0ff55 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -489,9 +489,10 @@ static int process_ops(struct filter_pred *preds,
 		if (!WARN_ON_ONCE(!pred->fn))
 			match = pred->fn(pred, rec);
 		if (!!match == type)
-			return match;
+			break;
 	}
-	return match;
+	/* If not of not match is equal to not of not, then it is a match */
+	return !!match == !op->not;
 }
 
 struct filter_match_preds_data {
@@ -740,10 +741,10 @@ static int filter_set_pred(struct event_filter *filter,
 		 * then this op can be folded.
 		 */
 		if (left->index & FILTER_PRED_FOLD &&
-		    (left->op == dest->op ||
+		    ((left->op == dest->op && !left->not) ||
 		     left->left == FILTER_PRED_INVALID) &&
 		    right->index & FILTER_PRED_FOLD &&
-		    (right->op == dest->op ||
+		    ((right->op == dest->op && !right->not) ||
 		     right->left == FILTER_PRED_INVALID))
 			dest->index |= FILTER_PRED_FOLD;
 
-- 
2.1.3

[PATCH 3/3] ftracetests: Add test to test event filter logic

[Steven Rostedt]

[-- Attachment #1: 0003-ftracetests-Add-test-to-test-event-filter-logic.patch --]
[-- Type: text/plain, Size: 5411 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Add a test to test the event filter logic. It currently tests the
following filters against sched:sched_switch event.

   ( prev_pid != 0 )
   ( prev_pid == 0 )
   ( prev_pid < 100 )
   ( prev_pid <= $$ )
   ( prev_pid > 100 )
   ( prev_pid >= $$ )
  ! ( prev_pid != 0 )
  ! ( prev_pid == 0 )
  ! ( prev_pid < 100 )
  ! ( prev_pid <= $$ )
  ! ( prev_pid > 100 )
  ! ( prev_pid >= $$ )
   ( prev_pid != 0 && next_pid > 10 )
   ( prev_pid != 0 || next_pid > 10 )
  ! ( prev_pid != 0 && next_pid > 10 )
  ! ( prev_pid != 0 || next_pid > 10 )
   ( prev_pid & 1 )
   ( prev_pid & 2 )
   ( prev_pid & 4 )
   ( prev_pid & 8 )
   ( prev_pid & 16 )
  ! ( prev_pid & 1 )
  ! ( prev_pid & 2 )
  ! ( prev_pid & 4 )
  ! ( prev_pid & 8 )
  ! ( prev_pid & 16 )
   ( next_comm ~ "ftrace-test-fil" )
   ( next_comm != "ftrace-test-fil" )
  ! ( next_comm ~ "ftrace-test-fil" )
  ! ( next_comm != "ftrace-test-fil" )

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 .../selftests/ftrace/test.d/ftrace/filter.tc       | 200 +++++++++++++++++++++
 1 file changed, 200 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/filter.tc

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/filter.tc b/tools/testing/selftests/ftrace/test.d/ftrace/filter.tc
new file mode 100644
index 000000000000..de4c5ce57099
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/filter.tc
@@ -0,0 +1,200 @@
+#!/bin/sh
+# description: ftrace - test filter logic
+
+EVENT=events/sched/sched_switch
+FILTER=$EVENT/filter
+ENABLE=$EVENT/enable
+
+FIELD1=prev_pid
+FIELD2=next_pid
+FIELD3=next_comm
+
+ITER=100
+
+enable_event() {
+    echo 1 > $ENABLE
+}
+
+disable_event() {
+    echo 0 > $ENABLE
+}
+
+clear_filter() {
+    echo 0 > $FILTER
+}
+
+do_reset() {
+    enable_tracing
+    disable_event
+    clear_filter
+    clear_trace
+}
+
+fail() { # msg
+    do_reset
+    echo $1
+    exit -1
+}
+
+run_code() {
+    # make lots of pids to filter on
+    for i in `seq $ITER`; do
+	ls /usr > /dev/null
+    done
+}
+
+run_event() {
+    enable_event
+    run_code
+    disable_event
+}
+
+disable_tracing
+clear_trace
+enable_tracing
+
+test_cmp() {
+    cmp=$1
+    rval=$2
+    filter=$3
+    not=$4
+
+    echo "$not ( $FIELD1 $filter )"
+    echo "$not ( $FIELD1 $filter )" > $FILTER
+
+    run_event
+
+    cat trace | sed -ne "s/.*$FIELD1=\([^ ]*\).*/\1/p" |
+    while read pid; do
+	if [ "$pid" $cmp $rval ]; then
+	    if [ "$not" == '!' ]; then
+		fail "$pid matched filter: $filter; but should not not have for $not"
+	    fi
+	elif [ "$not" != '!' ]; then 
+	    fail "$pid does not match filter: $filter"
+	fi
+    done
+    clear_trace
+}
+
+test_cmp2() {
+    cmp1=$1
+    rval1=$2
+    conj=$3
+    cmp2=$4
+    rval2=$5
+    filter1=$6
+    filter2=$7
+    fconj=$8
+    not=$9
+
+    echo "$not ( $FIELD1 $filter1 $fconj $FIELD2 $filter2 )"
+    echo "$not ( $FIELD1 $filter1 $fconj $FIELD2 $filter2 )" > $FILTER
+
+    run_event
+
+    cat trace | sed -ne "s/.*$FIELD1=\([^ ]*\).*$FIELD2=\([^ ]*\).*/\1 \2/p" |
+    while read pid1 pid2; do
+	if [ "$pid1" $cmp1 $rval1 $conj "$pid2" $cmp2 $rval2 ]; then
+	    if [ "$not" == '!' ]; then
+		fail "$pid1 and $pid2 match not filter: $filter1 $fconj $filter2"
+	    fi
+	elif [ "$not" != '!' ]; then
+	    fail "$pid1 and $pid2 does not match filter: $filter1 $fconj $filter2"
+	fi
+    done
+    clear_trace
+}
+
+# first simple compares
+test_cmp -ne 0 "!= 0" ""
+test_cmp -eq 0 "== 0" ""
+test_cmp -lt 100 "< 100" ""
+test_cmp -le $$ "<= $$" ""
+test_cmp -gt 100 "> 100" ""
+test_cmp -ge $$ ">= $$" ""
+
+# Now test the not case
+test_cmp -ne 0 "!= 0" '!'
+test_cmp -eq 0 "== 0" '!'
+test_cmp -lt 100 "< 100" '!'
+test_cmp -le $$ "<= $$" '!'
+test_cmp -gt 100 "> 100" '!'
+test_cmp -ge $$ ">= $$" '!'
+
+# Test more complex compares (&& and !!)
+test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
+test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''
+
+test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" '!'
+test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" '!'
+
+# test bitmask
+
+bit_test() {
+    not=$1
+
+    let x=1
+    for i in `seq 5`; do
+	echo "$not ( $FIELD1 & $x )"
+	echo "$not ( $FIELD1 & $x )" > $FILTER
+	run_event
+
+	cat trace | sed -ne "s/.*$FIELD1=\([^ ]*\).*/\1/p" |
+	while read pid; do
+
+	    # do not exit if val becomes zero
+	    set +e
+	    let val="$pid & $x"
+	    set -e
+
+	    if [ $val -eq 0 ]; then
+		if [ "$not" != '!' ]; then
+		    fail "$pid did not match bitmask $x"
+		fi
+	    elif [ "$not" == '!' ]; then
+		fail "$pid did not match not bitmask $x"
+	    fi
+	done
+	clear_trace
+	let x="$x << 1"
+    done
+}		
+
+bit_test ''
+bit_test '!'
+
+test_string() {
+    cmp=$1
+    filter=$2
+    not=$3
+
+    comm=`cat /proc/$$/comm`
+
+    echo "$not ( $FIELD3 $filter \"$comm\" )"
+    echo "$not ( $FIELD3 $filter \"$comm\" )" > $FILTER
+
+    run_event
+
+    cat trace | sed -ne "s/.*$FIELD3=\([^ ]*\).*/\1/p" |
+    while read com; do
+	if [ "$com" $cmp "$comm" ]; then
+	    if [ "$not" == '!' ]; then
+		fail "$com matched filter: $filter; but should not not have for $not"
+	    fi
+	elif [ "$not" != '!' ]; then 
+	    fail "$com does not match filter: $filter"
+	fi
+    done
+    clear_trace
+}
+
+test_string "==" "~" ''
+test_string "!=" "!=" ''
+
+test_string "==" "~" '!'
+test_string "!=" "!=" '!'
+
+do_reset
+
+exit 0
-- 
2.1.3

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Alexei Starovoitov]

On Tue, Dec 2, 2014 at 7:13 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
> Add a test to test the event filter logic. It currently tests the
> following filters against sched:sched_switch event.
>
>    ( prev_pid != 0 )
>    ( prev_pid == 0 )
>    ( prev_pid < 100 )
>    ( prev_pid <= $$ )
>    ( prev_pid > 100 )
>    ( prev_pid >= $$ )
>   ! ( prev_pid != 0 )
>   ! ( prev_pid == 0 )
>   ! ( prev_pid < 100 )
>   ! ( prev_pid <= $$ )
>   ! ( prev_pid > 100 )
>   ! ( prev_pid >= $$ )
>    ( prev_pid != 0 && next_pid > 10 )
>    ( prev_pid != 0 || next_pid > 10 )
>   ! ( prev_pid != 0 && next_pid > 10 )
>   ! ( prev_pid != 0 || next_pid > 10 )
>    ( prev_pid & 1 )
>    ( prev_pid & 2 )
>    ( prev_pid & 4 )
>    ( prev_pid & 8 )
>    ( prev_pid & 16 )
>   ! ( prev_pid & 1 )
>   ! ( prev_pid & 2 )
>   ! ( prev_pid & 4 )
>   ! ( prev_pid & 8 )
>   ! ( prev_pid & 16 )
>    ( next_comm ~ "ftrace-test-fil" )
>    ( next_comm != "ftrace-test-fil" )
>   ! ( next_comm ~ "ftrace-test-fil" )
>   ! ( next_comm != "ftrace-test-fil" )

thanks for the tests!
Since you're adding full support for 'not',
I think would be good to have few more tests
where ! is not a top node. Like:
(prev_pid != 0 && !(next_pid & 2))
and another one with ! at multiple levels, like:
(prev_pid != 0 && !(next_pid != 2 && !(prev_pid > 3)))
... or reject them during parsing.

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Steven Rostedt]

On Tue, 2 Dec 2014 22:22:16 -0800
Alexei Starovoitov <ast@plumgrid.com> wrote:

> On Tue, Dec 2, 2014 at 7:13 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> >
> > Add a test to test the event filter logic. It currently tests the
> > following filters against sched:sched_switch event.
> >
> >    ( prev_pid != 0 )
> >    ( prev_pid == 0 )
> >    ( prev_pid < 100 )
> >    ( prev_pid <= $$ )
> >    ( prev_pid > 100 )
> >    ( prev_pid >= $$ )
> >   ! ( prev_pid != 0 )
> >   ! ( prev_pid == 0 )
> >   ! ( prev_pid < 100 )
> >   ! ( prev_pid <= $$ )
> >   ! ( prev_pid > 100 )
> >   ! ( prev_pid >= $$ )
> >    ( prev_pid != 0 && next_pid > 10 )
> >    ( prev_pid != 0 || next_pid > 10 )
> >   ! ( prev_pid != 0 && next_pid > 10 )
> >   ! ( prev_pid != 0 || next_pid > 10 )
> >    ( prev_pid & 1 )
> >    ( prev_pid & 2 )
> >    ( prev_pid & 4 )
> >    ( prev_pid & 8 )
> >    ( prev_pid & 16 )
> >   ! ( prev_pid & 1 )
> >   ! ( prev_pid & 2 )
> >   ! ( prev_pid & 4 )
> >   ! ( prev_pid & 8 )
> >   ! ( prev_pid & 16 )
> >    ( next_comm ~ "ftrace-test-fil" )
> >    ( next_comm != "ftrace-test-fil" )
> >   ! ( next_comm ~ "ftrace-test-fil" )
> >   ! ( next_comm != "ftrace-test-fil" )
> 
> thanks for the tests!
> Since you're adding full support for 'not',
> I think would be good to have few more tests
> where ! is not a top node. Like:
> (prev_pid != 0 && !(next_pid & 2))
> and another one with ! at multiple levels, like:
> (prev_pid != 0 && !(next_pid != 2 && !(prev_pid > 3)))
> ... or reject them during parsing.

Sure, would you like to add them :-)

I spent more time on this than I should have. I have other things to
work on and this little project has already put me behind on my other
tasks.

-- Steve

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Masami Hiramatsu]

(2014/12/03 12:13), Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Add a test to test the event filter logic. It currently tests the
> following filters against sched:sched_switch event.
> 
>    ( prev_pid != 0 )
>    ( prev_pid == 0 )
>    ( prev_pid < 100 )
>    ( prev_pid <= $$ )
>    ( prev_pid > 100 )
>    ( prev_pid >= $$ )
>   ! ( prev_pid != 0 )
>   ! ( prev_pid == 0 )
>   ! ( prev_pid < 100 )
>   ! ( prev_pid <= $$ )
>   ! ( prev_pid > 100 )
>   ! ( prev_pid >= $$ )
>    ( prev_pid != 0 && next_pid > 10 )
>    ( prev_pid != 0 || next_pid > 10 )
>   ! ( prev_pid != 0 && next_pid > 10 )
>   ! ( prev_pid != 0 || next_pid > 10 )
>    ( prev_pid & 1 )
>    ( prev_pid & 2 )
>    ( prev_pid & 4 )
>    ( prev_pid & 8 )
>    ( prev_pid & 16 )
>   ! ( prev_pid & 1 )
>   ! ( prev_pid & 2 )
>   ! ( prev_pid & 4 )
>   ! ( prev_pid & 8 )
>   ! ( prev_pid & 16 )
>    ( next_comm ~ "ftrace-test-fil" )
>    ( next_comm != "ftrace-test-fil" )
>   ! ( next_comm ~ "ftrace-test-fil" )
>   ! ( next_comm != "ftrace-test-fil" )
> 

Hmm, this uses some bash-only syntax, here is the result of checkbashisms.

# checkbashisms tools/testing/selftests/ftrace/test.d/ftrace/filter.tc
possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 70 (should be 'b = a'):
            if [ "$not" == '!' ]; then
possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 99 (should be 'b = a'):
            if [ "$not" == '!' ]; then
possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 137 (let ...):
    let x=1
possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 148 (let ...):
            let val="$pid & $x"
possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 155 (should be 'b = a'):
            elif [ "$not" == '!' ]; then
possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 160 (let ...):
        let x="$x << 1"
possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 182 (should be 'b = a'):
            if [ "$not" == '!' ]; then

to allow run this on busybox or dash, we'd better clean it.

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
[...]
> +# Test more complex compares (&& and !!)
> +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
> +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''

This might better be
test_cmp2 -ne 0 -a -gt 10 "!= 0" "&&" "> 10" ''
test_cmp2 -ne 0 -o -gt 10 "!= 0" "||" "> 10" ''

:-)

> +
> +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" '!'
> +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" '!'

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Steven Rostedt]

On Wed, 03 Dec 2014 18:26:43 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2014/12/03 12:13), Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > Add a test to test the event filter logic. It currently tests the
> > following filters against sched:sched_switch event.
> > 
> >    ( prev_pid != 0 )
> >    ( prev_pid == 0 )
> >    ( prev_pid < 100 )
> >    ( prev_pid <= $$ )
> >    ( prev_pid > 100 )
> >    ( prev_pid >= $$ )
> >   ! ( prev_pid != 0 )
> >   ! ( prev_pid == 0 )
> >   ! ( prev_pid < 100 )
> >   ! ( prev_pid <= $$ )
> >   ! ( prev_pid > 100 )
> >   ! ( prev_pid >= $$ )
> >    ( prev_pid != 0 && next_pid > 10 )
> >    ( prev_pid != 0 || next_pid > 10 )
> >   ! ( prev_pid != 0 && next_pid > 10 )
> >   ! ( prev_pid != 0 || next_pid > 10 )
> >    ( prev_pid & 1 )
> >    ( prev_pid & 2 )
> >    ( prev_pid & 4 )
> >    ( prev_pid & 8 )
> >    ( prev_pid & 16 )
> >   ! ( prev_pid & 1 )
> >   ! ( prev_pid & 2 )
> >   ! ( prev_pid & 4 )
> >   ! ( prev_pid & 8 )
> >   ! ( prev_pid & 16 )
> >    ( next_comm ~ "ftrace-test-fil" )
> >    ( next_comm != "ftrace-test-fil" )
> >   ! ( next_comm ~ "ftrace-test-fil" )
> >   ! ( next_comm != "ftrace-test-fil" )
> > 
> 
> Hmm, this uses some bash-only syntax, here is the result of checkbashisms.
> 
> # checkbashisms tools/testing/selftests/ftrace/test.d/ftrace/filter.tc
> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 70 (should be 'b = a'):
>             if [ "$not" == '!' ]; then
> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 99 (should be 'b = a'):
>             if [ "$not" == '!' ]; then
> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 137 (let ...):
>     let x=1
> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 148 (let ...):
>             let val="$pid & $x"
> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 155 (should be 'b = a'):
>             elif [ "$not" == '!' ]; then
> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 160 (let ...):
>         let x="$x << 1"
> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 182 (should be 'b = a'):
>             if [ "$not" == '!' ]; then
> 
> to allow run this on busybox or dash, we'd better clean it.

Do you know how to fix this?

> 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> [...]
> > +# Test more complex compares (&& and !!)
> > +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
> > +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''
> 
> This might better be
> test_cmp2 -ne 0 -a -gt 10 "!= 0" "&&" "> 10" ''
> test_cmp2 -ne 0 -o -gt 10 "!= 0" "||" "> 10" ''
> 
> :-)

Sure.

OK, I'll hold off on sending this patch then till 3.20.

I'll still add the update to the kernel for 3.19, but the testing for
it needs work. It passes my test suite, but I don't know how to handle
the busybox limitations.

-- Steve

> 
> > +
> > +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" '!'
> > +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" '!'
> 
> Thank you,
> 
> 

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Steven Rostedt]

On Wed, 3 Dec 2014 04:34:23 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
 
> OK, I'll hold off on sending this patch then till 3.20.
> 

BTW, can we add another option that just shows the echo output. The -d
enables -x which floods the screen with lots of useless information and
it's hard to see what is going on.

Thanks,

-- Steve

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Alexei Starovoitov]

On Wed, Dec 3, 2014 at 12:40 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2 Dec 2014 22:22:16 -0800
> Alexei Starovoitov <ast@plumgrid.com> wrote:
>
>> On Tue, Dec 2, 2014 at 7:13 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>> >
>> > Add a test to test the event filter logic. It currently tests the
>> > following filters against sched:sched_switch event.
>> >
>> >    ( prev_pid != 0 )
>> >    ( prev_pid == 0 )
>> >    ( prev_pid < 100 )
>> >    ( prev_pid <= $$ )
>> >    ( prev_pid > 100 )
>> >    ( prev_pid >= $$ )
>> >   ! ( prev_pid != 0 )
>> >   ! ( prev_pid == 0 )
>> >   ! ( prev_pid < 100 )
>> >   ! ( prev_pid <= $$ )
>> >   ! ( prev_pid > 100 )
>> >   ! ( prev_pid >= $$ )
>> >    ( prev_pid != 0 && next_pid > 10 )
>> >    ( prev_pid != 0 || next_pid > 10 )
>> >   ! ( prev_pid != 0 && next_pid > 10 )
>> >   ! ( prev_pid != 0 || next_pid > 10 )
>> >    ( prev_pid & 1 )
>> >    ( prev_pid & 2 )
>> >    ( prev_pid & 4 )
>> >    ( prev_pid & 8 )
>> >    ( prev_pid & 16 )
>> >   ! ( prev_pid & 1 )
>> >   ! ( prev_pid & 2 )
>> >   ! ( prev_pid & 4 )
>> >   ! ( prev_pid & 8 )
>> >   ! ( prev_pid & 16 )
>> >    ( next_comm ~ "ftrace-test-fil" )
>> >    ( next_comm != "ftrace-test-fil" )
>> >   ! ( next_comm ~ "ftrace-test-fil" )
>> >   ! ( next_comm != "ftrace-test-fil" )
>>
>> thanks for the tests!
>> Since you're adding full support for 'not',
>> I think would be good to have few more tests
>> where ! is not a top node. Like:
>> (prev_pid != 0 && !(next_pid & 2))
>> and another one with ! at multiple levels, like:
>> (prev_pid != 0 && !(next_pid != 2 && !(prev_pid > 3)))
>> ... or reject them during parsing.
>
> Sure, would you like to add them :-)
>
> I spent more time on this than I should have. I have other things to
> work on and this little project has already put me behind on my other
> tasks.

aren't we all starting to hack things only to realize
that the scope of work is too large? ;)
Anyway, will try to add these tests when your patches land.

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Steven Rostedt]

On Wed, 3 Dec 2014 09:53:18 -0800
Alexei Starovoitov <ast@plumgrid.com> wrote:

 
> aren't we all starting to hack things only to realize
> that the scope of work is too large? ;)

Yep. Although I did say I would only spend a few hours on a solution,
and would give up if it too longer. The patches themselves took much
shorter than I expected. It was writing the tests that brought me over
the brink. Although, I did a lot of manual tests to make sure things
did what was expected. But those manual tests need to become automatic
ones.


> Anyway, will try to add these tests when your patches land.

I'm placing the code changes in my 3.19 queue, and the test cases in my
3.20 queue. This is probably a good idea anyway, as the test cases will
fail if the code wasn't implemented, and the test goes in a separate
branch, where the code isn't implemented :-)  I did a merge to do the
test cases.

I also need to solve the busybox limitation. I don't know how that
shell can do bit logic, which is needed to verify that the filters
really did work.

-- Steve 

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Masami Hiramatsu]

(2014/12/03 18:34), Steven Rostedt wrote:
> On Wed, 03 Dec 2014 18:26:43 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> (2014/12/03 12:13), Steven Rostedt wrote:
>>> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>>>
>>> Add a test to test the event filter logic. It currently tests the
>>> following filters against sched:sched_switch event.
>>>
>>>    ( prev_pid != 0 )
>>>    ( prev_pid == 0 )
>>>    ( prev_pid < 100 )
>>>    ( prev_pid <= $$ )
>>>    ( prev_pid > 100 )
>>>    ( prev_pid >= $$ )
>>>   ! ( prev_pid != 0 )
>>>   ! ( prev_pid == 0 )
>>>   ! ( prev_pid < 100 )
>>>   ! ( prev_pid <= $$ )
>>>   ! ( prev_pid > 100 )
>>>   ! ( prev_pid >= $$ )
>>>    ( prev_pid != 0 && next_pid > 10 )
>>>    ( prev_pid != 0 || next_pid > 10 )
>>>   ! ( prev_pid != 0 && next_pid > 10 )
>>>   ! ( prev_pid != 0 || next_pid > 10 )
>>>    ( prev_pid & 1 )
>>>    ( prev_pid & 2 )
>>>    ( prev_pid & 4 )
>>>    ( prev_pid & 8 )
>>>    ( prev_pid & 16 )
>>>   ! ( prev_pid & 1 )
>>>   ! ( prev_pid & 2 )
>>>   ! ( prev_pid & 4 )
>>>   ! ( prev_pid & 8 )
>>>   ! ( prev_pid & 16 )
>>>    ( next_comm ~ "ftrace-test-fil" )
>>>    ( next_comm != "ftrace-test-fil" )
>>>   ! ( next_comm ~ "ftrace-test-fil" )
>>>   ! ( next_comm != "ftrace-test-fil" )
>>>
>>
>> Hmm, this uses some bash-only syntax, here is the result of checkbashisms.
>>
>> # checkbashisms tools/testing/selftests/ftrace/test.d/ftrace/filter.tc
>> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 70 (should be 'b = a'):
>>             if [ "$not" == '!' ]; then
>> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 99 (should be 'b = a'):
>>             if [ "$not" == '!' ]; then
>> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 137 (let ...):
>>     let x=1
>> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 148 (let ...):
>>             let val="$pid & $x"
>> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 155 (should be 'b = a'):
>>             elif [ "$not" == '!' ]; then
>> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 160 (let ...):
>>         let x="$x << 1"
>> possible bashism in tools/testing/selftests/ftrace/test.d/ftrace/filter.tc line 182 (should be 'b = a'):
>>             if [ "$not" == '!' ]; then
>>
>> to allow run this on busybox or dash, we'd better clean it.
> 
> Do you know how to fix this?

Yes, it's easy :).
As the tool output, [ a == b ] can be changed to [ a = b ],
and "let ..." can be changed to "$((...))"

> 
>>
>>> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>>> ---
>> [...]
>>> +# Test more complex compares (&& and !!)
>>> +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
>>> +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''
>>
>> This might better be
>> test_cmp2 -ne 0 -a -gt 10 "!= 0" "&&" "> 10" ''
>> test_cmp2 -ne 0 -o -gt 10 "!= 0" "||" "> 10" ''
>>
>> :-)
> 
> Sure.
> 
> OK, I'll hold off on sending this patch then till 3.20.

OK.

> 
> I'll still add the update to the kernel for 3.19, but the testing for
> it needs work. It passes my test suite, but I don't know how to handle
> the busybox limitations.

If you are using Fedora, it provides dash and busybox packages too.

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Steven Rostedt]

On Thu, 04 Dec 2014 08:14:01 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> >> to allow run this on busybox or dash, we'd better clean it.
> > 
> > Do you know how to fix this?
> 
> Yes, it's easy :).
> As the tool output, [ a == b ] can be changed to [ a = b ],
> and "let ..." can be changed to "$((...))"

OK, thanks.

> 
> > 
> >>
> >>> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> >>> ---
> >> [...]
> >>> +# Test more complex compares (&& and !!)
> >>> +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
> >>> +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''
> >>
> >> This might better be
> >> test_cmp2 -ne 0 -a -gt 10 "!= 0" "&&" "> 10" ''
> >> test_cmp2 -ne 0 -o -gt 10 "!= 0" "||" "> 10" ''
> >>
> >> :-)
> > 
> > Sure.
> > 
> > OK, I'll hold off on sending this patch then till 3.20.
> 
> OK.
> 
> > 
> > I'll still add the update to the kernel for 3.19, but the testing for
> > it needs work. It passes my test suite, but I don't know how to handle
> > the busybox limitations.
> 
> If you are using Fedora, it provides dash and busybox packages too.

Yep, and so does Debian. In fact, I notice that my Debian boxes all
have /bin/sh linked to dash not bash (which means they were not as
susceptible to the bash bug).

-- Steve

[PATCH v2] ftracetests: Add test to test event filter logic

[Steven Rostedt]

Add a test to test the event filter logic. It currently tests the
following filters against sched:sched_switch event.

   ( prev_pid != 0 )
   ( prev_pid == 0 )
   ( prev_pid < 100 )
   ( prev_pid <= $$ )
   ( prev_pid > 100 )
   ( prev_pid >= $$ )
  ! ( prev_pid != 0 )
  ! ( prev_pid == 0 )
  ! ( prev_pid < 100 )
  ! ( prev_pid <= $$ )
  ! ( prev_pid > 100 )
  ! ( prev_pid >= $$ )
   ( prev_pid != 0 && next_pid > 10 )
   ( prev_pid != 0 || next_pid > 10 )
  ! ( prev_pid != 0 && next_pid > 10 )
  ! ( prev_pid != 0 || next_pid > 10 )
   ( prev_pid & 1 )
   ( prev_pid & 2 )
   ( prev_pid & 4 )
   ( prev_pid & 8 )
   ( prev_pid & 16 )
  ! ( prev_pid & 1 )
  ! ( prev_pid & 2 )
  ! ( prev_pid & 4 )
  ! ( prev_pid & 8 )
  ! ( prev_pid & 16 )
   ( next_comm ~ "ftrace-test-fil" )
   ( next_comm != "ftrace-test-fil" )
  ! ( next_comm ~ "ftrace-test-fil" )
  ! ( next_comm != "ftrace-test-fil" )

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 .../selftests/ftrace/test.d/ftrace/filter.tc       | 197 +++++++++++++++++++++
 1 file changed, 197 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/filter.tc

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/filter.tc b/tools/testing/selftests/ftrace/test.d/ftrace/filter.tc
new file mode 100644
index 000000000000..eef7cc6320f2
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/filter.tc
@@ -0,0 +1,197 @@
+#!/bin/sh
+# description: ftrace - test filter logic
+
+EVENT=events/sched/sched_switch
+FILTER=$EVENT/filter
+ENABLE=$EVENT/enable
+
+FIELD1=prev_pid
+FIELD2=next_pid
+FIELD3=next_comm
+
+ITER=100
+
+enable_event() {
+    echo 1 > $ENABLE
+}
+
+disable_event() {
+    echo 0 > $ENABLE
+}
+
+clear_filter() {
+    echo 0 > $FILTER
+}
+
+do_reset() {
+    enable_tracing
+    disable_event
+    clear_filter
+    clear_trace
+}
+
+fail() { # msg
+    do_reset
+    echo $1
+    exit -1
+}
+
+run_code() {
+    # make lots of pids to filter on
+    for i in `seq $ITER`; do
+	ls /usr > /dev/null
+    done
+}
+
+run_event() {
+    enable_event
+    run_code
+    disable_event
+}
+
+disable_tracing
+clear_trace
+enable_tracing
+
+test_cmp() {
+    cmp=$1
+    rval=$2
+    filter=$3
+    not=$4
+
+    echo "$not ( $FIELD1 $filter )"
+    echo "$not ( $FIELD1 $filter )" > $FILTER
+
+    run_event
+
+    cat trace | sed -ne "s/.*$FIELD1=\([^ ]*\).*/\1/p" |
+    while read pid; do
+	if [ "$pid" $cmp $rval ]; then
+	    if [ "$not" = '!' ]; then
+		fail "$pid matched filter: $filter; but should not not have for $not"
+	    fi
+	elif [ "$not" != '!' ]; then 
+	    fail "$pid does not match filter: $filter"
+	fi
+    done
+    clear_trace
+}
+
+test_cmp2() {
+    cmp1=$1
+    rval1=$2
+    conj=$3
+    cmp2=$4
+    rval2=$5
+    filter1=$6
+    filter2=$7
+    fconj=$8
+    not=$9
+
+    echo "$not ( $FIELD1 $filter1 $fconj $FIELD2 $filter2 )"
+    echo "$not ( $FIELD1 $filter1 $fconj $FIELD2 $filter2 )" > $FILTER
+
+    run_event
+
+    cat trace | sed -ne "s/.*$FIELD1=\([^ ]*\).*$FIELD2=\([^ ]*\).*/\1 \2/p" |
+    while read pid1 pid2; do
+	if [ "$pid1" $cmp1 $rval1 $conj "$pid2" $cmp2 $rval2 ]; then
+	    if [ "$not" = '!' ]; then
+		fail "$pid1 and $pid2 match not filter: $filter1 $fconj $filter2"
+	    fi
+	elif [ "$not" != '!' ]; then
+	    fail "$pid1 and $pid2 does not match filter: $filter1 $fconj $filter2"
+	fi
+    done
+    clear_trace
+}
+
+# first simple compares
+test_cmp -ne 0 "!= 0" ""
+test_cmp -eq 0 "== 0" ""
+test_cmp -lt 100 "< 100" ""
+test_cmp -le $$ "<= $$" ""
+test_cmp -gt 100 "> 100" ""
+test_cmp -ge $$ ">= $$" ""
+
+# Now test the not case
+test_cmp -ne 0 "!= 0" '!'
+test_cmp -eq 0 "== 0" '!'
+test_cmp -lt 100 "< 100" '!'
+test_cmp -le $$ "<= $$" '!'
+test_cmp -gt 100 "> 100" '!'
+test_cmp -ge $$ ">= $$" '!'
+
+# Test more complex compares (&& and !!)
+test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
+test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''
+
+test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" '!'
+test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" '!'
+
+# test bitmask
+
+bit_test() {
+    not=$1
+
+    x=1
+    for i in `seq 5`; do
+	echo "$not ( $FIELD1 & $x )"
+	echo "$not ( $FIELD1 & $x )" > $FILTER
+	run_event
+
+	cat trace | sed -ne "s/.*$FIELD1=\([^ ]*\).*/\1/p" |
+	while read pid; do
+
+	    val=$(($pid & $x))
+
+	    if [ $val -eq 0 ]; then
+		if [ "$not" != '!' ]; then
+		    fail "$pid did not match bitmask $x"
+		fi
+	    elif [ "$not" = '!' ]; then
+		fail "$pid did not match not bitmask $x"
+	    fi
+	done
+	clear_trace
+	x=$(($x << 1))
+    done
+}		
+
+bit_test ''
+bit_test '!'
+
+test_string() {
+    cmp=$1
+    filter=$2
+    not=$3
+
+    comm=`cat /proc/$$/comm`
+
+    echo "$not ( $FIELD3 $filter \"$comm\" )"
+    echo "$not ( $FIELD3 $filter \"$comm\" )" > $FILTER
+
+    run_event
+
+    cat trace | sed -ne "s/.*$FIELD3=\([^ ]*\).*/\1/p" |
+    while read com; do
+	if [ "$com" $cmp "$comm" ]; then
+	    if [ "$not" = '!' ]; then
+		fail "$com matched filter: $filter; but should not not have for $not"
+	    fi
+	elif [ "$not" != '!' ]; then 
+	    fail "$com does not match filter: $filter"
+	fi
+    done
+    clear_trace
+}
+
+test_string "=" "~" ''
+test_string "!=" "!=" ''
+
+test_string "=" "~" '!'
+test_string "!=" "!=" '!'
+
+do_reset
+
+exit 0
-- 
1.8.1.4

Re: [PATCH 3/3] ftracetests: Add test to test event filter logic

[Masami Hiramatsu]

(2014/12/04 1:20), Steven Rostedt wrote:
> On Wed, 3 Dec 2014 04:34:23 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>  
>> OK, I'll hold off on sending this patch then till 3.20.
>>
> 
> BTW, can we add another option that just shows the echo output. The -d
> enables -x which floods the screen with lots of useless information and
> it's hard to see what is going on.

I see, actually -d is not for debugging each test case, but just
for the ftracetest itself.
I'll add --verbose to show echos in testcases.


Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH v2] ftracetests: Add test to test event filter logic

[Masami Hiramatsu]

Hi Steven,

I have just one comment.

(2014/12/04 12:18), Steven Rostedt wrote:
> +# Test more complex compares (&& and !!)
> +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
> +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''
> +
> +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" '!'
> +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" '!'

These are not fixed :(

> +    filter1=$6
> +    filter2=$7
> +    fconj=$8

are better to be

> +    filter1=$6
> +    fconj=$7
> +    filter2=$8

so that we can easily review parameters.

Others are OK for me.

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH v2] ftracetests: Add test to test event filter logic

[Steven Rostedt]

On Thu, 04 Dec 2014 19:06:37 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Hi Steven,
> 
> I have just one comment.
> 
> (2014/12/04 12:18), Steven Rostedt wrote:
> > +# Test more complex compares (&& and !!)
> > +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" ''
> > +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" ''
> > +
> > +test_cmp2 -ne 0 -a -gt 10 "!= 0" "> 10" "&&" '!'
> > +test_cmp2 -ne 0 -o -gt 10 "!= 0" "> 10" "||" '!'
> 
> These are not fixed :(

Oops, forgot about this. I was so focused on the dash commands I forgot
about the easy stuff.

> 
> > +    filter1=$6
> > +    filter2=$7
> > +    fconj=$8
> 
> are better to be
> 
> > +    filter1=$6
> > +    fconj=$7
> > +    filter2=$8
> 
> so that we can easily review parameters.
> 
> Others are OK for me.
> 

Yep, I'll make a v3 soon.

-- Steve