Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: 2024-08-30 14:58:08
Also in:
lkml
From: Menglong Dong <redacted> Date: Fri, 30 Aug 2024 09:59:55 +0800
quoted hunk ↗ jump to hunk
Change the return type of vxlan_set_mac() from bool to enum skb_drop_reason. In this commit, two drop reasons are introduced: VXLAN_DROP_INVALID_SMAC VXLAN_DROP_ENTRY_EXISTS To make it easier to document the reasons in drivers/net/vxlan/drop.h, we don't define the enum vxlan_drop_reason with the macro VXLAN_DROP_REASONS(), but hand by hand. Signed-off-by: Menglong Dong <redacted> --- drivers/net/vxlan/drop.h | 9 +++++++++ drivers/net/vxlan/vxlan_core.c | 12 ++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h index 6bcc6894fbbd..876b4a9de92f 100644 --- a/drivers/net/vxlan/drop.h +++ b/drivers/net/vxlan/drop.h@@ -9,11 +9,20 @@ #include <net/dropreason.h> #define VXLAN_DROP_REASONS(R) \ + R(VXLAN_DROP_INVALID_SMAC) \ + R(VXLAN_DROP_ENTRY_EXISTS) \
To Jakub: In our recent conversation, you said you dislike templates much. What about this one? :>
/* deliberate comment for trailing \ */
enum vxlan_drop_reason {
__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
SKB_DROP_REASON_SUBSYS_SHIFT,
+ /** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
+ VXLAN_DROP_INVALID_SMAC,
+ /**
+ * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
+ * one pointing to a nexthop
+ */Maybe you'd do a proper kdoc for this enum at the top?
quoted hunk ↗ jump to hunk
+ VXLAN_DROP_ENTRY_EXISTS, }; static inline voiddiff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c index 76b217d166ef..58c175432a15 100644 --- a/drivers/net/vxlan/vxlan_core.c +++ b/drivers/net/vxlan/vxlan_core.c@@ -1607,9 +1607,9 @@ static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed, unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS; } -static bool vxlan_set_mac(struct vxlan_dev *vxlan, - struct vxlan_sock *vs, - struct sk_buff *skb, __be32 vni) +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan, + struct vxlan_sock *vs, + struct sk_buff *skb, __be32 vni) { union vxlan_addr saddr; u32 ifindex = skb->dev->ifindex;@@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, /* Ignore packet loops (and multicast echo) */ if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr)) - return false; + return (u32)VXLAN_DROP_INVALID_SMAC; /* Get address from the outer IP header */ if (vxlan_get_sk_family(vs) == AF_INET) {@@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, if ((vxlan->cfg.flags & VXLAN_F_LEARN) && vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni)) - return false; + return (u32)VXLAN_DROP_ENTRY_EXISTS; - return true; + return (u32)SKB_NOT_DROPPED_YET;
Redundant cast.
}
Don't you need to adjust the call site accordingly? Previously, this function returned false in case of error and true otherwise, now it returns a non-zero in case of error and 0 (NOT_DROPPED_YET == 0) if everything went fine. IOW the call site now needs to check whether the return value != NOT_DROPPED_YET instead of checking whether it returned false. You inverted the return code logic, but didn't touch the call site.
static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
Thanks, Olek