[PATCH net-next 1/1] net sched actions: merge event notification routines

[PATCH net-next 1/1] net sched actions: merge event notification routines

[Roman Mashak]

Collapse tca_get_notify(), tca_add_notify() and tca_del_notify() in a
single function since they repeat the same code pattern.

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 net/sched/act_api.c | 111 ++++++++++++++++------------------------------------
 1 file changed, 33 insertions(+), 78 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 57cf37145282..5b04184fb525 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -895,24 +895,41 @@ static int tca_get_fill(struct sk_buff *skb, struct list_head *actions,
 	return -1;
 }
 
-static int
-tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
-	       struct list_head *actions, int event,
-	       struct netlink_ext_ack *extack)
+static int tca_notify(struct net *net, struct nlmsghdr *n,
+		      struct list_head *actions, u32 portid, int event,
+		      size_t attr_size, struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
+	int err;
 
-	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+	skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
+			GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
-	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
-			 0, 0) <= 0) {
-		NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action");
+
+	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq,
+			 event == RTM_NEWACTION ? n->nlmsg_flags : 0,
+			 event, 0,
+			 event == RTM_DELACTION ? 1 : 0) <= 0) {
+		NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action attributes in event message");
 		kfree_skb(skb);
 		return -EINVAL;
 	}
 
-	return rtnl_unicast(skb, net, portid);
+	if (event == RTM_GETACTION) {
+		return rtnl_unicast(skb, net, portid);
+	} else if (event == RTM_DELACTION) {
+		/* now do the delete */
+		err = tcf_action_destroy(actions, 0);
+		if (err < 0) {
+			NL_SET_ERR_MSG(extack, "Failed to delete TC action");
+			kfree_skb(skb);
+			return err;
+		}
+	}
+	err = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
+			     n->nlmsg_flags & NLM_F_ECHO);
+	return err > 0 ? 0 : err;
 }
 
 static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
@@ -1034,40 +1051,6 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 }
 
 static int
-tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
-	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
-{
-	int ret;
-	struct sk_buff *skb;
-
-	skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
-			GFP_KERNEL);
-	if (!skb)
-		return -ENOBUFS;
-
-	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
-			 0, 1) <= 0) {
-		NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action attributes");
-		kfree_skb(skb);
-		return -EINVAL;
-	}
-
-	/* now do the delete */
-	ret = tcf_action_destroy(actions, 0);
-	if (ret < 0) {
-		NL_SET_ERR_MSG(extack, "Failed to delete TC action");
-		kfree_skb(skb);
-		return ret;
-	}
-
-	ret = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
-			     n->nlmsg_flags & NLM_F_ECHO);
-	if (ret > 0)
-		return 0;
-	return ret;
-}
-
-static int
 tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	      u32 portid, int event, struct netlink_ext_ack *extack)
 {
@@ -1102,46 +1085,17 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 
 	attr_size = tcf_action_full_attrs_size(attr_size);
 
-	if (event == RTM_GETACTION)
-		ret = tcf_get_notify(net, portid, n, &actions, event, extack);
-	else { /* delete */
-		ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
-		if (ret)
-			goto err;
-		return ret;
-	}
+	ret = tca_notify(net, n, &actions, portid, event, attr_size, extack);
+	if (ret)
+		goto err;
+	return ret;
+
 err:
 	if (event != RTM_GETACTION)
 		tcf_action_destroy(&actions, 0);
 	return ret;
 }
 
-static int
-tcf_add_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
-	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
-{
-	struct sk_buff *skb;
-	int err = 0;
-
-	skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
-			GFP_KERNEL);
-	if (!skb)
-		return -ENOBUFS;
-
-	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, n->nlmsg_flags,
-			 RTM_NEWACTION, 0, 0) <= 0) {
-		NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action");
-		kfree_skb(skb);
-		return -EINVAL;
-	}
-
-	err = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
-			     n->nlmsg_flags & NLM_F_ECHO);
-	if (err > 0)
-		err = 0;
-	return err;
-}
-
 static int tcf_action_add(struct net *net, struct nlattr *nla,
 			  struct nlmsghdr *n, u32 portid, int ovr,
 			  struct netlink_ext_ack *extack)
@@ -1155,7 +1109,8 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 	if (ret)
 		return ret;
 
-	return tcf_add_notify(net, n, &actions, portid, attr_size, extack);
+	return tca_notify(net, n, &actions, portid, RTM_NEWACTION, attr_size,
+			  extack);
 }
 
 static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;
-- 
2.7.4

Re: [PATCH net-next 1/1] net sched actions: merge event notification routines

[David Miller]

From: Roman Mashak <mrv@mojatatu.com>
Date: Thu, 22 Mar 2018 18:00:25 -0400

> Collapse tca_get_notify(), tca_add_notify() and tca_del_notify() in a
> single function since they repeat the same code pattern.
> 
> Signed-off-by: Roman Mashak <mrv@mojatatu.com>

This really doesn't improve the situation.

Instead of a couple clearly coded helpers, you now have one which is
bombed with conditionals that change it's behavior based upon the
event that gets passed in.

This is much less intuitive than how the code is now.

I'm not applying this, sorry.