| Message ID | 20230228034955.1215122-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2] u32: fix TC_U32_TERMINAL printing | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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
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
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
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 ",
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(-)