IOS XR: policy map parsing #6432
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #6432 +/- ##
============================================
+ Coverage 73.11% 73.15% +0.04%
- Complexity 35305 35348 +43
============================================
Files 2819 2820 +1
Lines 142941 143066 +125
Branches 17214 17228 +14
============================================
+ Hits 104509 104665 +156
+ Misses 30168 30117 -51
- Partials 8264 8284 +20 |
There was a problem hiding this comment.
Reviewed 2 of 5 files at r1, 1 of 2 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ratulm)
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_xr/CiscoXr_qos.g4, line 772 at r3 (raw file):
DEC ( ACTIVATE DYNAMIC_TEMPLATE dtname = variable
These alternatives should be separate rules. The ones you ignore and/or don't flesh out can go in a null rule.
The dynamic-template one certainly should not be succeeded by null_rest_of_line based on your unit test. Separating that one out will also make the extractor cleaner, i.e. it won't have if-else-if chain based on alternative here.
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_xr/CiscoXr_qos.g4, line 871 at r3 (raw file):
POLICY_MAP ( mapname = variable NEWLINE pm_tail*
I suspect when you don't say the type there is a default type among those below.
I'd strongly prefer you make a rule for that default type that can either have or not have the declared type.
Then the rest of the ignored types can go in a much cleaner rule that doesn't have the option of no declared type.
Please split rules at first token at which decision is made.
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_xr/CiscoXr_qos.g4, line 884 at r3 (raw file):
| REDIRECT | TRAFFIC ) mapname = variable NEWLINE pm_tail*
I am really not a fan of having a pm_tail here that aggregates the ignored lines of all the different types. This stuff is far more maintainable if you spend a little time now making separate rules for each policy-map type, each of which has its own pm_xxx_null_tail tail rules (or whatever you want to call it). Also personally i usually inline the tail rules, but I am not sure whether that is currently better or worse for recovery. I'm sure @dhalperi has an opinion on this that also hedges on legibility.
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_xr/CiscoXr_qos.g4, line 887 at r3 (raw file):
) ) pm_end_policy_map?
Is this actually optional for XR? Sure it isn't always present/absent?
projects/batfish/src/main/java/org/batfish/representation/cisco_xr/CiscoXrStructureType.java, line 24 at r3 (raw file):
ICMP_TYPE_OBJECT_GROUP("object-group icmp-type"), INSPECT_CLASS_MAP("class-map type inspect"), INSPECT_POLICY_MAP("policy-map type inspect"),
Can you remove all the deleted structure types?
projects/batfish/src/test/resources/org/batfish/grammar/cisco_xr/testconfigs/policy-map, line 14 at r3 (raw file):
! class class-default class type control subscriber CM do-all
No def/ref tests for this?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel and @dhalperi)
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_xr/CiscoXr_qos.g4, line 884 at r3 (raw file):
Previously, arifogel (Ari Fogel) wrote…
I am really not a fan of having a
pm_tailhere that aggregates the ignored lines of all the different types. This stuff is far more maintainable if you spend a little time now making separate rules for each policy-map type, each of which has its ownpm_xxx_null_tailtail rules (or whatever you want to call it). Also personally i usually inline thetailrules, but I am not sure whether that is currently better or worse for recovery. I'm sure @dhalperi has an opinion on this that also hedges on legibility.
as far as i could tell the null tail is shared. so, i left it like that but the pointer to it goes via separate types.
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_xr/CiscoXr_qos.g4, line 887 at r3 (raw file):
Previously, arifogel (Ari Fogel) wrote…
Is this actually optional for XR? Sure it isn't always present/absent?
Yes, it is optional.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Lots of cruft gone. I'm sure there will be follow-on work, but 
Reviewable status:
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ratulm)
projects/batfish/src/test/resources/org/batfish/grammar/cisco_xr/testconfigs/policy-map, line 17 at r4 (raw file):
class type control subscriber PPP do-until-success ! ! end-policy-map
Since the end-block is omitted for the maps below, may as well not omit it here.
Fixes #6348
Looks like we had copied over qos parsing from uber-cisco-parser to the xr-parser. The syntax in the two worlds appear quite different (and there were no tests for qos parsing in XR). This PR 1) adds parsing for the policy-map syntax in the issue and fleshes out the neighborhood, and 2) removes the
s_policy_map_ios_inspectwhich doesn't appear valid for XR.