[PATCH iproute2] u32: fix TC_U32_TERMINAL printing

[PATCH iproute2] u32: fix TC_U32_TERMINAL printing

[Hangbin Liu]

We previously printed an asterisk if there was no 'sel' or 'TC_U32_TERMINAL'
flag. However, commit 1ff22754 ("u32: fix json formatting of flowid")
changed the logic to print an asterisk only if there is a 'TC_U32_TERMINAL'
flag. Therefore, we need to fix this regression.

Fixes: 1ff227545ce1 ("u32: fix json formatting of flowid")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tc/f_u32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index bfe9e5f9..de2d0c9e 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1273,7 +1273,7 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 	if (tb[TCA_U32_CLASSID]) {
 		__u32 classid = rta_getattr_u32(tb[TCA_U32_CLASSID]);
 		SPRINT_BUF(b1);
-		if (sel && (sel->flags & TC_U32_TERMINAL))
+		if (!sel || !(sel->flags & TC_U32_TERMINAL))
 			print_string(PRINT_FP, NULL, "*", NULL);
 
 		print_string(PRINT_ANY, "flowid", "flowid %s ",
-- 
2.38.1

Re: [PATCH iproute2] u32: fix TC_U32_TERMINAL printing

[Jamal Hadi Salim]

Hangbin,
Can you please run tdc tests on all tc (both for iproute2 and kernel)
changes you make and preferably show them in the commit log? If you
introduce something new then add a new tdc test case to cover it.

cheers,
jamal

On Mon, Feb 27, 2023 at 10:50 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> We previously printed an asterisk if there was no 'sel' or 'TC_U32_TERMINAL'
> flag. However, commit 1ff22754 ("u32: fix json formatting of flowid")
> changed the logic to print an asterisk only if there is a 'TC_U32_TERMINAL'
> flag. Therefore, we need to fix this regression.
>
> Fixes: 1ff227545ce1 ("u32: fix json formatting of flowid")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  tc/f_u32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tc/f_u32.c b/tc/f_u32.c
> index bfe9e5f9..de2d0c9e 100644
> --- a/tc/f_u32.c
> +++ b/tc/f_u32.c
> @@ -1273,7 +1273,7 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
>         if (tb[TCA_U32_CLASSID]) {
>                 __u32 classid = rta_getattr_u32(tb[TCA_U32_CLASSID]);
>                 SPRINT_BUF(b1);
> -               if (sel && (sel->flags & TC_U32_TERMINAL))
> +               if (!sel || !(sel->flags & TC_U32_TERMINAL))
>                         print_string(PRINT_FP, NULL, "*", NULL);
>
>                 print_string(PRINT_ANY, "flowid", "flowid %s ",
> --
> 2.38.1
>

Re: [PATCH iproute2] u32: fix TC_U32_TERMINAL printing

[Hangbin Liu]

On Tue, Feb 28, 2023 at 05:55:30AM -0500, Jamal Hadi Salim wrote:
> Hangbin,
> Can you please run tdc tests on all tc (both for iproute2 and kernel)
> changes you make and preferably show them in the commit log? If you
> introduce something new then add a new tdc test case to cover it.

OK, the patch fixed an issue I found when run tdc u32 test.

1..11
not ok 1 afa9 - Add u32 with source match
	Could not match regex pattern. Verify command output:
filter protocol ip pref 1 u32 chain 0 
filter protocol ip pref 1 u32 chain 0 fh 800: ht divisor 1 
filter protocol ip pref 1 u32 chain 0 fh 800::800 order 2048 key ht 800 bkt 0 *flowid 1:1 not_in_hw 
  match 7f000001/ffffffff at 12
	action order 1: gact action pass
	 random type none pass val 0
	 index 1 ref 1 bind 1

After the fix, the u32.json test passed

All test results:

1..11
ok 1 afa9 - Add u32 with source match
ok 2 6aa7 - Add/Replace u32 with source match and invalid indev
ok 3 bc4d - Replace valid u32 with source match and invalid indev
ok 4 648b - Add u32 with custom hash table
ok 5 6658 - Add/Replace u32 with custom hash table and invalid handle
ok 6 9d0a - Replace valid u32 with custom hash table and invalid handle
ok 7 1644 - Add u32 filter that links to a custom hash table
ok 8 74c2 - Add/Replace u32 filter with invalid hash table id
ok 9 1fe6 - Replace valid u32 filter with invalid hash table id
ok 10 0692 - Test u32 sample option, divisor 256
ok 11 2478 - Test u32 sample option, divisor 16


When I post the patch, I though this issue is a clear logic one, so I didn't
 paste the test result.

Thanks
Hangbin

Re: [PATCH iproute2] u32: fix TC_U32_TERMINAL printing

[Hangbin Liu]

On Wed, Mar 01, 2023 at 11:10:30AM +0800, Hangbin Liu wrote:
> On Tue, Feb 28, 2023 at 05:55:30AM -0500, Jamal Hadi Salim wrote:
> > Hangbin,
> > Can you please run tdc tests on all tc (both for iproute2 and kernel)
> > changes you make and preferably show them in the commit log? If you
> > introduce something new then add a new tdc test case to cover it.
> 
> OK, the patch fixed an issue I found when run tdc u32 test.
> 
> 1..11
> not ok 1 afa9 - Add u32 with source match
> 	Could not match regex pattern. Verify command output:
> filter protocol ip pref 1 u32 chain 0 
> filter protocol ip pref 1 u32 chain 0 fh 800: ht divisor 1 
> filter protocol ip pref 1 u32 chain 0 fh 800::800 order 2048 key ht 800 bkt 0 *flowid 1:1 not_in_hw 
>   match 7f000001/ffffffff at 12
> 	action order 1: gact action pass
> 	 random type none pass val 0
> 	 index 1 ref 1 bind 1
> 
> After the fix, the u32.json test passed
> 
> All test results:
> 
> 1..11
> ok 1 afa9 - Add u32 with source match
> ok 2 6aa7 - Add/Replace u32 with source match and invalid indev
> ok 3 bc4d - Replace valid u32 with source match and invalid indev
> ok 4 648b - Add u32 with custom hash table
> ok 5 6658 - Add/Replace u32 with custom hash table and invalid handle
> ok 6 9d0a - Replace valid u32 with custom hash table and invalid handle
> ok 7 1644 - Add u32 filter that links to a custom hash table
> ok 8 74c2 - Add/Replace u32 filter with invalid hash table id
> ok 9 1fe6 - Replace valid u32 filter with invalid hash table id
> ok 10 0692 - Test u32 sample option, divisor 256
> ok 11 2478 - Test u32 sample option, divisor 16
> 
> 
> When I post the patch, I though this issue is a clear logic one, so I didn't
>  paste the test result.

Should I re-post the patch?

Hangbin

Re: [PATCH iproute2] u32: fix TC_U32_TERMINAL printing

[Jamal Hadi Salim]

On Tue, Feb 28, 2023 at 10:10 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Tue, Feb 28, 2023 at 05:55:30AM -0500, Jamal Hadi Salim wrote:
> > Hangbin,
> > Can you please run tdc tests on all tc (both for iproute2 and kernel)
> > changes you make and preferably show them in the commit log? If you
> > introduce something new then add a new tdc test case to cover it.
>
> OK, the patch fixed an issue I found when run tdc u32 test.
>
> 1..11
> not ok 1 afa9 - Add u32 with source match
>         Could not match regex pattern. Verify command output:
> filter protocol ip pref 1 u32 chain 0
> filter protocol ip pref 1 u32 chain 0 fh 800: ht divisor 1
> filter protocol ip pref 1 u32 chain 0 fh 800::800 order 2048 key ht 800 bkt 0 *flowid 1:1 not_in_hw
>   match 7f000001/ffffffff at 12
>         action order 1: gact action pass
>          random type none pass val 0
>          index 1 ref 1 bind 1
>
> After the fix, the u32.json test passed
>
> All test results:
>
> 1..11
> ok 1 afa9 - Add u32 with source match
> ok 2 6aa7 - Add/Replace u32 with source match and invalid indev
> ok 3 bc4d - Replace valid u32 with source match and invalid indev
> ok 4 648b - Add u32 with custom hash table
> ok 5 6658 - Add/Replace u32 with custom hash table and invalid handle
> ok 6 9d0a - Replace valid u32 with custom hash table and invalid handle
> ok 7 1644 - Add u32 filter that links to a custom hash table
> ok 8 74c2 - Add/Replace u32 filter with invalid hash table id
> ok 9 1fe6 - Replace valid u32 filter with invalid hash table id
> ok 10 0692 - Test u32 sample option, divisor 256
> ok 11 2478 - Test u32 sample option, divisor 16
>
>
> When I post the patch, I though this issue is a clear logic one, so I didn't
>  paste the test result.
>

I think we should make it a rule to run tdc going forward. Your last
submission's issue that Pedro fixed would have been trivially found
with tdc. My suggestion is to repost the patch including the tdc test
in the commit message.

cheers,
jamal

> Thanks
> Hangbin