Message ID | 20240212155519.106206-3-amusil@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | Remove most of the hardcoded table numbers | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | warning | apply and check: warning |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Thanks Ales, Acked-by: Mark Michelson <mmichels@redhat.com> On 2/12/24 10:55, Ales Musil wrote: > To avoid issues with hardcoded table numbers in future add rule > into check patch. The rule is only warning because there are still > legitimate use cases and not everything can be abstracted. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v4: Rebase on top of main. > Address comments from Dumitru: > - Fix the regex. > - Add test for the new check. > --- > tests/checkpatch.at | 39 +++++++++++++++++++++++++++++++++++++++ > utilities/checkpatch.py | 12 ++++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/tests/checkpatch.at b/tests/checkpatch.at > index e7322fff4..6ac0e51f3 100755 > --- a/tests/checkpatch.at > +++ b/tests/checkpatch.at > @@ -605,3 +605,42 @@ try_checkpatch \ > Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!" > > AT_CLEANUP > + > +AT_SETUP([checkpatch - hardcoded table numbers]) > +try_checkpatch \ > + "COMMON_PATCH_HEADER([tests/something.at]) > + +table=12(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=??);) > + " \ > + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. > + #8 FILE: tests/something.at:1: > + table=12(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=??);) > +" > + > +try_checkpatch \ > + "COMMON_PATCH_HEADER([tests/something.at]) > + +table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=13);) > + " \ > + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. > + #8 FILE: tests/something.at:1: > + table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=13);) > +" > + > +try_checkpatch \ > + "COMMON_PATCH_HEADER([tests/something.at]) > + +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47) > + " \ > + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. > + #8 FILE: tests/something.at:1: > + priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47) > +" > + > +try_checkpatch \ > + "COMMON_PATCH_HEADER([tests/something.at]) > + +C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep "dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]]) > + " \ > + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. > + #8 FILE: tests/something.at:1: > + C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep "dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]]) > +" > + > +AT_CLEANUP > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > index 52d3fa845..35204daa2 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -202,6 +202,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' % > __parenthesized_constructs) > __regex_nonascii_characters = re.compile("[^\u0000-\u007f]") > __regex_efgrep = re.compile(r'.*[ef]grep.*$') > +__regex_hardcoded_table = re.compile(r'.*(table=[0-9]+)|.*(resubmit\(,[0-9]+\))') > > skip_leading_whitespace_check = False > skip_trailing_whitespace_check = False > @@ -371,6 +372,10 @@ def has_efgrep(line): > """Returns TRUE if the current line contains 'egrep' or 'fgrep'.""" > return __regex_efgrep.match(line) is not None > > +def has_hardcoded_table(line): > + """Return TRUE if the current line contains table=<NUMBER> or > + resubmit(,<NUMBER>)""" > + return __regex_hardcoded_table.match(line) is not None > > def filter_comments(current_line, keep=False): > """remove all of the c-style comments in a line""" > @@ -656,6 +661,13 @@ checks = [ > 'check': lambda x: has_efgrep(x), > 'print': > lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")}, > + > + {'regex': r'\.at$', 'match_name': None, > + 'check': lambda x: has_hardcoded_table(x), > + 'print': > + lambda: print_warning("Use of hardcoded table=<NUMBER> or" > + " resubmit=(,<NUMBER>) is discouraged in tests." > + " Consider using MACRO instead.")}, > ] > >
diff --git a/tests/checkpatch.at b/tests/checkpatch.at index e7322fff4..6ac0e51f3 100755 --- a/tests/checkpatch.at +++ b/tests/checkpatch.at @@ -605,3 +605,42 @@ try_checkpatch \ Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!" AT_CLEANUP + +AT_SETUP([checkpatch - hardcoded table numbers]) +try_checkpatch \ + "COMMON_PATCH_HEADER([tests/something.at]) + +table=12(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=??);) + " \ + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. + #8 FILE: tests/something.at:1: + table=12(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=??);) +" + +try_checkpatch \ + "COMMON_PATCH_HEADER([tests/something.at]) + +table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=13);) + " \ + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. + #8 FILE: tests/something.at:1: + table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=13);) +" + +try_checkpatch \ + "COMMON_PATCH_HEADER([tests/something.at]) + +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47) + " \ + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. + #8 FILE: tests/something.at:1: + priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47) +" + +try_checkpatch \ + "COMMON_PATCH_HEADER([tests/something.at]) + +C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep "dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]]) + " \ + "WARNING: Use of hardcoded table=<NUMBER> or resubmit=(,<NUMBER>) is discouraged in tests. Consider using MACRO instead. + #8 FILE: tests/something.at:1: + C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep "dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]]) +" + +AT_CLEANUP diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 52d3fa845..35204daa2 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -202,6 +202,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' % __parenthesized_constructs) __regex_nonascii_characters = re.compile("[^\u0000-\u007f]") __regex_efgrep = re.compile(r'.*[ef]grep.*$') +__regex_hardcoded_table = re.compile(r'.*(table=[0-9]+)|.*(resubmit\(,[0-9]+\))') skip_leading_whitespace_check = False skip_trailing_whitespace_check = False @@ -371,6 +372,10 @@ def has_efgrep(line): """Returns TRUE if the current line contains 'egrep' or 'fgrep'.""" return __regex_efgrep.match(line) is not None +def has_hardcoded_table(line): + """Return TRUE if the current line contains table=<NUMBER> or + resubmit(,<NUMBER>)""" + return __regex_hardcoded_table.match(line) is not None def filter_comments(current_line, keep=False): """remove all of the c-style comments in a line""" @@ -656,6 +661,13 @@ checks = [ 'check': lambda x: has_efgrep(x), 'print': lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")}, + + {'regex': r'\.at$', 'match_name': None, + 'check': lambda x: has_hardcoded_table(x), + 'print': + lambda: print_warning("Use of hardcoded table=<NUMBER> or" + " resubmit=(,<NUMBER>) is discouraged in tests." + " Consider using MACRO instead.")}, ]
To avoid issues with hardcoded table numbers in future add rule into check patch. The rule is only warning because there are still legitimate use cases and not everything can be abstracted. Signed-off-by: Ales Musil <amusil@redhat.com> --- v4: Rebase on top of main. Address comments from Dumitru: - Fix the regex. - Add test for the new check. --- tests/checkpatch.at | 39 +++++++++++++++++++++++++++++++++++++++ utilities/checkpatch.py | 12 ++++++++++++ 2 files changed, 51 insertions(+)