Re: [ovs-dev] [RFC net-next 4/7] selftests: openvswitch: Add support for tunnel() key.

Simon Horman horms@kernel.org writes:

On Thu, Jun 13, 2024 at 02:13:30PM -0400, Aaron Conole wrote:

...

This will be used when setting details about the tunnel to use as transport. There is a difference between the ODP format between tunnel(): the 'key' flag is not actually a flag field, so we don't support it in the same way that the vswitchd userspace supports displaying it.

Signed-off-by: Aaron Conole aconole@redhat.com

...

...

@@ -1265,6 +1265,165 @@ class ovskey(nla): init=init, )

• class ovs_key_tunnel(nla):

•    nla_flags = NLA_F_NESTED
  

•    nla_map = (
  

•        ("OVS_TUNNEL_KEY_ATTR_ID", "be64"),
  

•        ("OVS_TUNNEL_KEY_ATTR_IPV4_SRC", "ipaddr"),
  

•        ("OVS_TUNNEL_KEY_ATTR_IPV4_DST", "ipaddr"),
  

•        ("OVS_TUNNEL_KEY_ATTR_TOS", "uint8"),
  

•        ("OVS_TUNNEL_KEY_ATTR_TTL", "uint8"),
  

•        ("OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT", "flag"),
  

•        ("OVS_TUNNEL_KEY_ATTR_CSUM", "flag"),
  

•        ("OVS_TUNNEL_KEY_ATTR_OAM", "flag"),
  

•        ("OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS", "array(uint32)"),
  

•        ("OVS_TUNNEL_KEY_ATTR_TP_SRC", "be16"),
  

•        ("OVS_TUNNEL_KEY_ATTR_TP_DST", "be16"),
  

•        ("OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS", "none"),
  

•        ("OVS_TUNNEL_KEY_ATTR_IPV6_SRC", "ipaddr"),
  

•        ("OVS_TUNNEL_KEY_ATTR_IPV6_DST", "ipaddr"),
  

•        ("OVS_TUNNEL_KEY_ATTR_PAD", "none"),
  

•        ("OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS", "none"),
  

•        ("OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE", "flag"),
  

•    )
  

•    def parse(self, flowstr, mask=None):
  

•        if not flowstr.startswith("tunnel("):
  

•            return None, None
  

•        k = ovskey.ovs_key_tunnel()
  

•        if mask is not None:
  

•            mask = ovskey.ovs_key_tunnel()
  

•        flowstr = flowstr[len("tunnel("):]
  

•        v6_address = None
  

•        fields = [
  

•            ("tun_id=", r"(\d+)", int, "OVS_TUNNEL_KEY_ATTR_ID",
  

•             0xffffffffffffffff, None, None),
  

•            ("src=", r"([0-9a-fA-F\.]+)", str,
  

•             "OVS_TUNNEL_KEY_ATTR_IPV4_SRC", "255.255.255.255", "0.0.0.0",
  

•             False),
  

•            ("dst=", r"([0-9a-fA-F\.]+)", str,
  

•             "OVS_TUNNEL_KEY_ATTR_IPV4_DST", "255.255.255.255", "0.0.0.0",
  

•             False),
  

•            ("ipv6_src=", r"([0-9a-fA-F:]+)", str,
  

•             "OVS_TUNNEL_KEY_ATTR_IPV6_SRC",
  

•             "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", "::", True),
  

•            ("ipv6_dst=", r"([0-9a-fA-F:]+)", str,
  

•             "OVS_TUNNEL_KEY_ATTR_IPV6_DST",
  

•             "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", "::", True),
  

•            ("tos=", r"(\d+)", int, "OVS_TUNNEL_KEY_ATTR_TOS", 255, 0,
  

•             None),
  

•            ("ttl=", r"(\d+)", int, "OVS_TUNNEL_KEY_ATTR_TTL", 255, 0,
  

•             None),
  

•            ("tp_src=", r"(\d+)", int, "OVS_TUNNEL_KEY_ATTR_TP_SRC",
  

•             65535, 0, None),
  

•            ("tp_dst=", r"(\d+)", int, "OVS_TUNNEL_KEY_ATTR_TP_DST",
  

•             65535, 0, None),
  

•        ]
  

•        forced_include = ["OVS_TUNNEL_KEY_ATTR_TTL"]
  

•        for prefix, regex, typ, attr_name, mask_val, default_val, v46_flag in fields:
  

•            flowstr, value = parse_extract_field(flowstr, prefix, regex, typ, False)
  

•            if not attr_name:
  

•                raise Exception("Bad list value in tunnel fields")
  

•            if value is None and attr_name in forced_include:
  

•                value = default_val
  

•                mask_val = default_val
  

•            if value is not None:
  

•                if v6_address is None and v46_flag is not None:
  

•                    v6_address = v46_flag
  

By my reading, at this point v6_address will only be None if v46_flag is not None. IF so, the condition below seems excessive.

Agreed - thanks for the suggestions.

...

•                if v6_address is not None and v46_flag is not None \
  

•                   and v46_flag != v6_address:
  

•                    raise ValueError("Cannot mix v6 and v4 addresses")
  

I wonder if we can instead express this as (completely untested!):

                if v46_flag is not None:
                    if v6_address is None:
                        v6_address = v46_flag
                    if v46_flag != v6_address:
                        raise ValueError("Cannot mix v6 and v4 addresses")

...

•                k["attrs"].append([attr_name, value])
  

•                if mask is not None:
  

•                    mask["attrs"].append([attr_name, mask_val])
  

•            else:
  

•                if v6_address is not None and v46_flag is not None \
  

•                   and v46_flag != v6_address:
  

•                    continue
  

•                if v6_address is None and v46_flag is not None:
  

•                    continue
  

And I wonder if this is a bit easier on the eyes (also completely untested):

                if v46_flag is not None:
                    if v6_address is None or v46_flag != v6_address:
                        continue

I folded both of these in and did some quick testing. Thanks Simon!

...

•                if mask is not None:
  

•                    mask["attrs"].append([attr_name, default_val])
  

•        if k["attrs"][0][0] != "OVS_TUNNEL_KEY_ATTR_ID":
  

•            raise ValueError("Needs a tunid set")
  

...

...

@@ -1745,7 +1905,7 @@ class OvsVport(GenericNetlinkSocket): ) TUNNEL_DEFAULTS = [("geneve", 6081),

•                       ("vxlan", 4798)]
  

•                       ("vxlan", 4789)]
  

for tnl in TUNNEL_DEFAULTS: if ptype == tnl[0]:

As noted in my response to PATCH 1/7, I think that the change in the hunk above belongs there rather than here.