Hi Aaron, Thanks for the review!
A v4 is already posted at: https://lore.kernel.org/netdev/[email protected]/ It addresses several issues found during review (see changelog in the cover letter). Specifically for your comments: > ^ That's kindof a fix, BUT it also seems like we never actually used it > anywhere, so I hope it is okay to not treat it that way. Agreed! Since it was never used, treating it as a new feature is fine. In v4 the ENCAP type is changed to "encap_ovskey" (a new subclass) instead of "nested", which restricts the nla_map to L2-L4 attributes only. This avoids pyroute2 trying to decode metadata attributes (SKB_MARK, DP_HASH, etc.) that never appear inside ENCAP. > It would be more useful if we could set the MAX_ENCAP_DEPTH separately, > so that we have the option to break assumptions. Maybe just skip a > depth check here completely. Makes sense! The kernel already enforces its own nesting limits, so duplicating that check in the test tool is unnecessary. I'll drop the depth check in v5. Best, Minxi Aaron Conole <[email protected]> 于2026年5月4日周一 23:43写道： > > Minxi Hou <[email protected]> writes: > > > Extend the ovs-dpctl.py flow parser to support vlan() and encap() > > match strings. vlan() accepts tci=, vid=, pcp=, and cfi= > > parameters and generates the OVS_KEY_ATTR_VLAN attribute with a TCI > > value in network byte order. encap() parses nested flow strings > > and returns OVS_KEY_ATTR_ENCAP with inner key attributes as a > > recursive NLA container. > > > > The encap nla_map type is changed from "none" to "nested" so that > > pyroute2 recursively encodes the inner flow key attributes. The > > VLAN nla_map type is changed from "uint16" to "be16" to match the > > kernel's big-endian wire format. > > ^ That's kindof a fix, BUT it also seems like we never actually used it > anywhere, so I hope it is okay to not treat it that way. > > > Signed-off-by: Minxi Hou <[email protected]> > > --- > > v1 -> v2: rebase to latest net-next/main, drop --base=auto > > > > .../selftests/net/openvswitch/ovs-dpctl.py | 190 +++++++++++++++++- > > 1 file changed, 188 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > > b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > > index 848f61fdcee0..317be7878937 100644 > > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > > @@ -901,11 +901,11 @@ class ovskey(nla): > > nla_flags = NLA_F_NESTED > > nla_map = ( > > ("OVS_KEY_ATTR_UNSPEC", "none"), > > - ("OVS_KEY_ATTR_ENCAP", "none"), > > + ("OVS_KEY_ATTR_ENCAP", "nested"), > > ("OVS_KEY_ATTR_PRIORITY", "uint32"), > > ("OVS_KEY_ATTR_IN_PORT", "uint32"), > > ("OVS_KEY_ATTR_ETHERNET", "ethaddr"), > > - ("OVS_KEY_ATTR_VLAN", "uint16"), > > + ("OVS_KEY_ATTR_VLAN", "be16"), > > ("OVS_KEY_ATTR_ETHERTYPE", "be16"), > > ("OVS_KEY_ATTR_IPV4", "ovs_key_ipv4"), > > ("OVS_KEY_ATTR_IPV6", "ovs_key_ipv6"), > > @@ -1636,6 +1636,180 @@ class ovskey(nla): > > class ovs_key_mpls(nla): > > fields = (("lse", ">I"),) > > > > + # 802.1Q CFI (Canonical Format Indicator) bit, always set for Ethernet > > + _VLAN_CFI_MASK = 0x1000 > > + _MAX_ENCAP_DEPTH = 4 > > It would be more useful if we could set the MAX_ENCAP_DEPTH separately, > so that we have the option to break assumptions. Maybe just skip a > depth check here completely. > > > + _encap_depth = 0 # single-threaded usage assumed > > + > > + @staticmethod > > + def _parse_vlan_from_flowstr(flowstr): > > + """Parse vlan(tci=X) or vlan(vid=X[,pcp=Y,cfi=Z]) from flowstr. > > + > > + Returns (remaining_flowstr, key_tci, mask_tci). > > + TCI values use standard bit layout (VID bits 0-11, > > + CFI bit 12, PCP bits 13-15); byte order conversion to > > + big-endian happens in pyroute2 be16 NLA serialization. > > + The mask covers only the fields the caller specified: > > + vid -> 0x0FFF, pcp -> 0xE000, cfi -> 0x1000, tci -> 0xFFFF. > > + > > + The tci= key sets the raw TCI bitfield (no CFI validation) to allow > > + non-Ethernet use cases. Use cfi=1 for standard Ethernet VLAN > > matching. > > + """ > > + tci = 0 > > + mask = 0 > > + has_tci = False > > + has_vid = has_pcp = has_cfi = False > > + _tci_mix_err = "vlan(): 'tci' cannot be mixed " \ > > + "with 'vid'/'pcp'/'cfi'" > > + first = True > > + while True: > > + flowstr = flowstr.lstrip() > > + if not flowstr: > > + raise ValueError("vlan(): missing ')'") > > + if flowstr[0] == ')': > > + break > > + if not first: > > + flowstr = flowstr[1:] # skip ',' > > + if not flowstr: > > + raise ValueError("vlan(): missing ')' after trailing > > comma") > > + flowstr = flowstr.lstrip() > > + if flowstr and flowstr[0] == ')': > > + break > > + if flowstr and flowstr[0] == ',': > > + raise ValueError( > > + "vlan(): empty or extra comma in field list") > > + first = False > > + > > + eq = flowstr.find('=') > > + if eq == -1: > > + raise ValueError("vlan(): expected key=value, got '%s'" % > > flowstr) > > + key = flowstr[:eq].strip() > > + flowstr = flowstr[eq + 1:] > > + > > + end = flowstr.find(',') > > + end2 = flowstr.find(')') > > + if end == -1 or (end2 != -1 and end2 < end): > > + end = end2 > > + val = flowstr[:end].strip() > > + flowstr = flowstr[end:] > > + > > + if not val: > > + raise ValueError("vlan(): empty value for key '%s'" % key) > > + try: > > + v = int(val, 16) if val.startswith(('0x', '0X')) else > > int(val) > > + except ValueError: > > + raise ValueError("vlan(): invalid value '%s' for key '%s'" > > % > > + (val, key)) > > + > > + if key == 'tci': > > + if has_tci: > > + raise ValueError("vlan(): duplicate 'tci'") > > + if has_vid or has_pcp or has_cfi: > > + raise ValueError(_tci_mix_err) > > + if v > 0xFFFF or v < 0: > > + raise ValueError("vlan(): tci=0x%x out of range" % v) > > + tci = v > > + mask = 0xFFFF > > + has_tci = True > > + elif key == 'vid': > > + if has_tci: > > + raise ValueError(_tci_mix_err) > > + if has_vid: > > + raise ValueError("vlan(): duplicate 'vid'") > > + if v < 0 or v > 0xFFF: > > + raise ValueError("vlan(): vid=%d out of range > > (0-4095)" % v) > > + tci |= v > > + mask |= 0x0FFF > > + has_vid = True > > + elif key == 'pcp': > > + if has_tci: > > + raise ValueError(_tci_mix_err) > > + if has_pcp: > > + raise ValueError("vlan(): duplicate 'pcp'") > > + if v < 0 or v > 7: > > + raise ValueError("vlan(): pcp=%d out of range (0-7)" % > > v) > > + tci |= (v & 0x7) << 13 > > + mask |= 0xE000 > > + has_pcp = True > > + elif key == 'cfi': > > + if has_tci: > > + raise ValueError(_tci_mix_err) > > + if has_cfi: > > + raise ValueError("vlan(): duplicate 'cfi'") > > + if v != 1: > > + raise ValueError("vlan(): cfi must be 1 for Ethernet") > > + tci |= ovskey._VLAN_CFI_MASK > > + mask |= ovskey._VLAN_CFI_MASK > > + has_cfi = True > > + else: > > + raise ValueError("vlan(): unknown key '%s'" % key) > > + > > + flowstr = flowstr[1:] # skip ')' > > + # Catch immediate '))' (user error). A ')' after ',' is consumed > > + # by parse()'s strspn(flowstr, "), ") inter-field separator > > stripping. > > + if flowstr.lstrip().startswith(')'): > > + raise ValueError("vlan(): unmatched ')'") > > + # parse() strips trailing ',', ')', ' ' as inter-field separators, > > + # so we do not need to call strspn here. > > + > > + if mask == 0: > > + raise ValueError("vlan(): no fields specified, " > > + "use vlan(vid=X[,pcp=Y,cfi=Z]) or > > vlan(tci=X)") > > + if not has_tci: > > + tci |= ovskey._VLAN_CFI_MASK > > + mask |= ovskey._VLAN_CFI_MASK > > + return flowstr, tci, mask > > + > > + @staticmethod > > + def _parse_encap_from_flowstr(flowstr): > > + """Parse encap(inner_flow) from flowstr. > > + > > + Returns (remaining_flowstr, inner_key_dict, inner_mask_dict) > > + where each dict has an 'attrs' key for recursive NLA encoding. > > + Parenthesis-depth tracking handles nested encap() calls but not > > + quoted strings containing literal parentheses. > > + """ > > + if ovskey._encap_depth >= ovskey._MAX_ENCAP_DEPTH: > > + raise ValueError("encap(): max nesting depth %d exceeded" % > > + ovskey._MAX_ENCAP_DEPTH) > > + try: > > + ovskey._encap_depth += 1 > > + depth = 1 > > + end = -1 > > + for i, c in enumerate(flowstr): > > + if c == '(': > > + depth += 1 > > + elif c == ')': > > + depth -= 1 > > + if depth < 0: > > + raise ValueError("encap(): unmatched ')' at > > position %d" % i) > > + if depth == 0: > > + end = i > > + break > > + > > + if end == -1: > > + if depth > 1: > > + raise ValueError("encap(): missing ')' at end") > > + raise ValueError("encap(): missing closing ')'") > > + > > + inner_str = flowstr[:end].strip() > > + if not inner_str: > > + raise ValueError("encap(): empty inner flow") > > + > > + flowstr = flowstr[end + 1:] > > + if flowstr.lstrip().startswith(')'): > > + raise ValueError("encap(): unmatched ')' after encap()") > > + # parse() strips trailing ',', ')', ' ' as inter-field > > separators, > > + # so we do not need to call strspn here. > > + > > + inner_key = ovskey() > > + inner_mask = ovskey() > > + inner_key.parse(inner_str, inner_mask) > > + > > + return flowstr, inner_key, inner_mask > > + finally: > > + ovskey._encap_depth -= 1 > > + > > def parse(self, flowstr, mask=None): > > for field in ( > > ("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse), > > @@ -1657,6 +1831,16 @@ class ovskey(nla): > > "eth_type", > > lambda x: intparse(x, "0xffff"), > > ), > > + ( > > + "OVS_KEY_ATTR_VLAN", > > + "vlan", > > + ovskey._parse_vlan_from_flowstr, > > + ), > > + ( > > + "OVS_KEY_ATTR_ENCAP", > > + "encap", > > + ovskey._parse_encap_from_flowstr, > > + ), > > ( > > "OVS_KEY_ATTR_IPV4", > > "ipv4", > > @@ -1794,6 +1978,8 @@ class ovskey(nla): > > True, > > ), > > ("OVS_KEY_ATTR_ETHERNET", None, None, False, False), > > + ("OVS_KEY_ATTR_VLAN", "vlan", "0x%04x", lambda x: False, True), > > + ("OVS_KEY_ATTR_ENCAP", None, None, False, False), > > ( > > "OVS_KEY_ATTR_ETHERTYPE", > > "eth_type", > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
