Re: [PATCH net-next v5 1/2] selftests: openvswitch: add vlan() and encap() flow string parsing

[Aaron Conole <aconole@redhat.com>]

Minxi Hou <houminxi@gmail.com> writes:

> Add VLAN TCI formatting and parsing support to ovs-dpctl.py:
>
> - Add _vlan_dpstr() to decompose TCI into vid/pcp/cfi fields,
>   with raw tci=0x%04x fallback when cfi=0 for round-trip safety.
> - Add _parse_vlan_from_flowstr() boundary check for missing ')'.
> - Add encap_ovskey subclass restricting nla_map to L2-L4 attributes
>   (slots 0-21) that appear inside 802.1Q ENCAP, with metadata
>   attributes set to "none".
> - Check parse() return value for unrecognized trailing content.
> - Support callable format functions in dpstr() output.
> - Add push_vlan action class with fields matching kernel struct
>   ovs_action_push_vlan (vlan_tpid, vlan_tci as network-order u16).
> - Add push_vlan dpstr format and parse with range validation
>   (vid 0-4095, pcp 0-7, tpid 0-0xFFFF) and CFI forced to 1.
> - Remove MAX_ENCAP_DEPTH constant and depth tracking — the
>   bracket-depth counter in the encap parser already handles
>   nesting; the global depth limit was unnecessary.
>
> Signed-off-by: Minxi Hou <houminxi@gmail.com>
> ---

It's worth noting that there are pylint errors introduced with this
patch.  HOWEVER, they are related to things like pascal case (where
ovs-dpctl.py prefers snake_case), and using explicit string formatting
vs f-string.  I think it is okay to ignore these errors since it keeps
the file consistent, and it may be worth cleaning some of those up
(missing docstring and f-string conversion) at another time.

>  .../selftests/net/openvswitch/ovs-dpctl.py    | 322 +++++++++++++++++-
>  1 file changed, 312 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 848f61fdcee0..50551d4fa7c7 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -370,7 +370,7 @@ class ovsactions(nla):
>          ("OVS_ACTION_ATTR_OUTPUT", "uint32"),
>          ("OVS_ACTION_ATTR_USERSPACE", "userspace"),
>          ("OVS_ACTION_ATTR_SET", "ovskey"),
> -        ("OVS_ACTION_ATTR_PUSH_VLAN", "none"),
> +        ("OVS_ACTION_ATTR_PUSH_VLAN", "push_vlan"),
>          ("OVS_ACTION_ATTR_POP_VLAN", "flag"),
>          ("OVS_ACTION_ATTR_SAMPLE", "sample"),
>          ("OVS_ACTION_ATTR_RECIRC", "uint32"),
> @@ -427,6 +427,9 @@ class ovsactions(nla):
>  
>              return actstr
>  
> +    class push_vlan(nla):
> +        fields = (("vlan_tpid", "!H"), ("vlan_tci", "!H"))
> +
>      class sample(nla):
>          nla_flags = NLA_F_NESTED
>  
> @@ -633,6 +636,14 @@ class ovsactions(nla):
>                  print_str += "ct_clear"
>              elif field[0] == "OVS_ACTION_ATTR_POP_VLAN":
>                  print_str += "pop_vlan"
> +            elif field[0] == "OVS_ACTION_ATTR_PUSH_VLAN":
> +                datum = self.get_attr(field[0])
> +                tpid = datum["vlan_tpid"]
> +                tci = datum["vlan_tci"]
> +                vid = tci & 0x0FFF
> +                pcp = (tci >> 13) & 0x7
> +                print_str += "push_vlan(vid=%d,pcp=%d" \
> +                    ",tpid=0x%04x)" % (vid, pcp, tpid)
>              elif field[0] == "OVS_ACTION_ATTR_POP_ETH":
>                  print_str += "pop_eth"
>              elif field[0] == "OVS_ACTION_ATTR_POP_NSH":
> @@ -726,7 +737,57 @@ class ovsactions(nla):
>                      actstr = actstr[strspn(actstr, ", ") :]
>                      parsed = True
>  
> -            if parse_starts_block(actstr, "clone(", False):
> +            if parse_starts_block(actstr, "push_vlan(", False):
> +                actstr = actstr[len("push_vlan("):]
> +                vid = 0
> +                pcp = 0
> +                tpid = 0x8100
> +                if ")" not in actstr:
> +                    raise ValueError(
> +                        "push_vlan: missing ')'")
> +                paren = actstr.index(")")
> +                if not actstr[:paren].strip():
> +                    raise ValueError("push_vlan: no fields")
> +                for kv in actstr[:paren].split(","):
> +                    if "=" not in kv:
> +                        raise ValueError(
> +                            "push_vlan: bad field '%s'"
> +                            % kv.strip())
> +                    k = kv[:kv.index("=")].strip()
> +                    v = kv[kv.index("=") + 1:].strip()
> +                    if k == "vid":
> +                        vid = int(v, 0)
> +                        if vid < 0 or vid > 0xFFF:
> +                            raise ValueError(
> +                                "push_vlan: vid=%d out of "
> +                                "range (0-4095)" % vid)
> +                    elif k == "pcp":
> +                        pcp = int(v, 0)
> +                        if pcp < 0 or pcp > 7:
> +                            raise ValueError(
> +                                "push_vlan: pcp=%d out of "
> +                                "range (0-7)" % pcp)
> +                    elif k == "tpid":
> +                        tpid = int(v, 0)
> +                        if tpid < 0 or tpid > 0xFFFF:
> +                            raise ValueError(
> +                                "push_vlan: tpid=0x%x out "
> +                                "of range (0-0xffff)" % tpid)
> +                    else:
> +                        raise ValueError(
> +                            "push_vlan: unknown key '%s'"
> +                            % k)
> +                tci = (vid & 0x0FFF) | ((pcp & 0x7) << 13) \
> +                    | 0x1000
> +                pvact = self.push_vlan()
> +                pvact["vlan_tpid"] = tpid
> +                pvact["vlan_tci"] = tci
> +                self["attrs"].append(
> +                    ["OVS_ACTION_ATTR_PUSH_VLAN", pvact])
> +                actstr = actstr[paren + 1:]
> +                parsed = True
> +
> +            elif parse_starts_block(actstr, "clone(", False):
>                  parencount += 1
>                  subacts = ovsactions()
>                  actstr = actstr[len("clone("):]
> @@ -901,11 +962,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", "encap_ovskey"),
>          ("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 +1697,194 @@ 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
> +
> +    @staticmethod
> +    def _vlan_dpstr(tci):
> +        """Format VLAN TCI as vid=X,pcp=Y,cfi=Z or tci=0xNNNN.
> +
> +        When cfi=1 (standard Ethernet VLAN), outputs decomposed
> +        vid/pcp/cfi fields. When cfi=0 (truncated VLAN header),
> +        falls back to raw tci=0x%04x to ensure round-trip
> +        correctness: the parser auto-adds cfi=1 for vid/pcp
> +        format, so cfi=0 would be lost on re-parse."""
> +        vid = tci & 0x0FFF
> +        pcp = (tci >> 13) & 0x7
> +        cfi = (tci >> 12) & 0x1
> +        if cfi:
> +            return "vid=%d,pcp=%d,cfi=%d" % (vid, pcp, cfi)
> +        return "tci=0x%04x" % tci
> +
> +    @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 and end2 == -1:
> +                raise ValueError("vlan(): missing ')'")
> +            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 as exc:
> +                raise ValueError(
> +                    "vlan(): invalid value '%s' for key '%s'"
> +                    % (val, key)) from exc
> +
> +            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.
> +        """
> +        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()")
> +
> +        inner_key = encap_ovskey()
> +        inner_mask = encap_ovskey()
> +        remaining = inner_key.parse(inner_str, inner_mask)
> +        if remaining and re.search(r'[^\s,)]', remaining):
> +            raise ValueError(
> +                "encap(): unrecognized trailing "
> +                "content '%s'" % remaining.strip())
> +
> +        return flowstr, inner_key, inner_mask
> +
>      def parse(self, flowstr, mask=None):
>          for field in (
>              ("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),
> @@ -1657,6 +1906,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 +2053,9 @@ class ovskey(nla):
>                  True,
>              ),
>              ("OVS_KEY_ATTR_ETHERNET", None, None, False, False),
> +            ("OVS_KEY_ATTR_VLAN", "vlan", ovskey._vlan_dpstr,
> +                lambda x: False, True),
> +            ("OVS_KEY_ATTR_ENCAP", None, None, False, False),
>              (
>                  "OVS_KEY_ATTR_ETHERTYPE",
>                  "eth_type",
> @@ -1821,22 +2083,61 @@ class ovskey(nla):
>              v = self.get_attr(field[0])
>              if v is not None:
>                  m = None if mask is None else mask.get_attr(field[0])
> +                fmt = field[2]  # str format or callable
>                  if field[4] is False:
>                      print_str += v.dpstr(m, more)
>                      print_str += ","
>                  else:
>                      if m is None or field[3](m):
> -                        print_str += field[1] + "("
> -                        print_str += field[2] % v
> -                        print_str += "),"
> +                        val = fmt(v) if callable(fmt) else fmt % v
> +                        print_str += field[1] + "(" + val + "),"
>                      elif more or m != 0:
> -                        print_str += field[1] + "("
> -                        print_str += (field[2] % v) + "/" + (field[2] % m)
> -                        print_str += "),"
> +                        if callable(fmt):
> +                            val = fmt(v) + "/" + fmt(m)
> +                        else:
> +                            val = (fmt % v) + "/" + (fmt % m)
> +                        print_str += field[1] + "(" + val + "),"
>  
>          return print_str
>  
>  
> +class encap_ovskey(ovskey):
> +    """Inner flow key attributes valid inside 802.1Q ENCAP.
> +
> +    Only L2-L4 key attributes (slots 0-21) appear inside ENCAP.
> +    Metadata-only attributes (SKB_MARK, DP_HASH, RECIRC_ID, etc.)
> +    are set to "none" — they never appear inside ENCAP per
> +    ovs_nla_put_vlan() in net/openvswitch/flow_netlink.c.
> +
> +    nla_map indexes must match OVS_KEY_ATTR_* enum values in
> +    include/uapi/linux/openvswitch.h.
> +    """
> +    nla_map = (

I was thinking that we might be able to use something like:

  nla_map = ovskey.nlamap

But the comment clearly describes why we have this separate structure.
That said:

> +        ("OVS_KEY_ATTR_UNSPEC", "none"),       # 0
> +        ("OVS_KEY_ATTR_ENCAP", "none"),        # 1 — placeholder, no recursion
> +        ("OVS_KEY_ATTR_PRIORITY", "none"),       # 2 — skb metadata, not in ENCAP
> +        ("OVS_KEY_ATTR_IN_PORT", "none"),       # 3 — skb metadata, not in ENCAP

I don't think the '# <num>' are useful.  It may be useful to just
indicate the ones that are different from the ovskey base type, but not
include the number (looks like excess noise).

Otherwise, looks good to me.

> +        ("OVS_KEY_ATTR_ETHERNET", "ethaddr"),   # 4
> +        ("OVS_KEY_ATTR_VLAN", "be16"),          # 5
> +        ("OVS_KEY_ATTR_ETHERTYPE", "be16"),     # 6
> +        ("OVS_KEY_ATTR_IPV4", "ovs_key_ipv4"),  # 7
> +        ("OVS_KEY_ATTR_IPV6", "ovs_key_ipv6"),  # 8
> +        ("OVS_KEY_ATTR_TCP", "ovs_key_tcp"),    # 9
> +        ("OVS_KEY_ATTR_UDP", "ovs_key_udp"),    # 10
> +        ("OVS_KEY_ATTR_ICMP", "ovs_key_icmp"),  # 11
> +        ("OVS_KEY_ATTR_ICMPV6", "ovs_key_icmpv6"),  # 12
> +        ("OVS_KEY_ATTR_ARP", "ovs_key_arp"),    # 13
> +        ("OVS_KEY_ATTR_ND", "ovs_key_nd"),      # 14
> +        ("OVS_KEY_ATTR_SKB_MARK", "none"),      # 15 — metadata, not in ENCAP
> +        ("OVS_KEY_ATTR_TUNNEL", "none"),        # 16 — tunnel metadata, not in ENCAP
> +        ("OVS_KEY_ATTR_SCTP", "ovs_key_sctp"),  # 17
> +        ("OVS_KEY_ATTR_TCP_FLAGS", "be16"),     # 18
> +        ("OVS_KEY_ATTR_DP_HASH", "none"),       # 19 — metadata, not in ENCAP
> +        ("OVS_KEY_ATTR_RECIRC_ID", "none"),     # 20 — metadata, not in ENCAP
> +        ("OVS_KEY_ATTR_MPLS", "array(ovs_key_mpls)"),  # 21
> +    )
> +
> +
>  class OvsPacket(GenericNetlinkSocket):
>      OVS_PACKET_CMD_MISS = 1  # Flow table miss
>      OVS_PACKET_CMD_ACTION = 2  # USERSPACE action
> @@ -2576,6 +2877,7 @@ def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
>  
>  
>  def main(argv):
> +    nlmsg_atoms.encap_ovskey = encap_ovskey
>      nlmsg_atoms.ovskey = ovskey
>      nlmsg_atoms.ovsactions = ovsactions