[PATCH 0/5] net_sched: actions - Add default lookup and walker

[PATCH 0/5] net_sched: actions - Add default lookup and walker

[Jamal Hadi Salim]

This set of patches provide defaults for lookup and walkers for actions.
Users can override when needed.


Jamal Hadi Salim (5):
  Default action lookup method for actions that dont provide one
  Use default action lookup functions Users are free to override
  Provide default walker function for actions
  Use default action walker methods Users can still override
  Fail if missing mandatory action operation methods

 net/sched/act_api.c     |   27 +++++++++++++++------------
 net/sched/act_csum.c    |    2 --
 net/sched/act_gact.c    |    2 --
 net/sched/act_ipt.c     |    4 ----
 net/sched/act_mirred.c  |    2 --
 net/sched/act_nat.c     |    2 --
 net/sched/act_pedit.c   |    2 --
 net/sched/act_police.c  |    1 -
 net/sched/act_simple.c  |    1 -
 net/sched/act_skbedit.c |    1 -
 10 files changed, 15 insertions(+), 29 deletions(-)

-- 
1.7.10.4

[PATCH 1/5] net_sched: Default action lookup method for actions

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index fd70728..053a115 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -279,6 +279,10 @@ int tcf_register_action(struct tc_action_ops *act)
 	}
 	act->next = NULL;
 	*ap = act;
+
+	if (!act->lookup)
+		act->lookup = tcf_hash_search;
+
 	write_unlock(&act_mod_lock);
 	return 0;
 }
@@ -723,8 +727,6 @@ tcf_action_get_1(struct nlattr *nla, struct nlmsghdr *n, u32 portid)
 	a->ops = tc_lookup_action(tb[TCA_ACT_KIND]);
 	if (a->ops == NULL)
 		goto err_free;
-	if (a->ops->lookup == NULL)
-		goto err_mod;
 	err = -ENOENT;
 	if (a->ops->lookup(a, index) == 0)
 		goto err_mod;
-- 
1.7.10.4

[PATCH 2/5] net_sched: Use default action lookup functions

[Jamal Hadi Salim]

 Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_csum.c   |    1 -
 net/sched/act_gact.c   |    1 -
 net/sched/act_ipt.c    |    2 --
 net/sched/act_mirred.c |    1 -
 net/sched/act_nat.c    |    1 -
 net/sched/act_pedit.c  |    1 -
 net/sched/act_police.c |    1 -
 7 files changed, 8 deletions(-)

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 3a4c0ca..4225a93 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -585,7 +585,6 @@ static struct tc_action_ops act_csum_ops = {
 	.act		= tcf_csum,
 	.dump		= tcf_csum_dump,
 	.cleanup	= tcf_csum_cleanup,
-	.lookup		= tcf_hash_search,
 	.init		= tcf_csum_init,
 	.walk		= tcf_generic_walker
 };
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index fd2b3cf..15851da 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -206,7 +206,6 @@ static struct tc_action_ops act_gact_ops = {
 	.act		=	tcf_gact,
 	.dump		=	tcf_gact_dump,
 	.cleanup	=	tcf_gact_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_gact_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 60d88b6..1d3e191 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -298,7 +298,6 @@ static struct tc_action_ops act_ipt_ops = {
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_ipt_init,
 	.walk		=	tcf_generic_walker
 };
@@ -312,7 +311,6 @@ static struct tc_action_ops act_xt_ops = {
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_ipt_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 977c10e..6cb16ec 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -271,7 +271,6 @@ static struct tc_action_ops act_mirred_ops = {
 	.act		=	tcf_mirred,
 	.dump		=	tcf_mirred_dump,
 	.cleanup	=	tcf_mirred_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_mirred_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 876f0ef..30c13de 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -308,7 +308,6 @@ static struct tc_action_ops act_nat_ops = {
 	.act		=	tcf_nat,
 	.dump		=	tcf_nat_dump,
 	.cleanup	=	tcf_nat_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_nat_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 7ed78c9..ab4fc56 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -243,7 +243,6 @@ static struct tc_action_ops act_pedit_ops = {
 	.act		=	tcf_pedit,
 	.dump		=	tcf_pedit_dump,
 	.cleanup	=	tcf_pedit_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_pedit_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 272d8e9..16a62c3 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -407,7 +407,6 @@ static struct tc_action_ops act_police_ops = {
 	.act		=	tcf_act_police,
 	.dump		=	tcf_act_police_dump,
 	.cleanup	=	tcf_act_police_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_act_police_locate,
 	.walk		=	tcf_act_police_walker
 };
-- 
1.7.10.4

[PATCH 3/5] net_sched: Provide default walker function for actions

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 053a115..401eca8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -280,8 +280,11 @@ int tcf_register_action(struct tc_action_ops *act)
 	act->next = NULL;
 	*ap = act;
 
+	/* Supply defaults */
 	if (!act->lookup)
 		act->lookup = tcf_hash_search;
+	if (!act->walk)
+		act->walk = tcf_generic_walker;
 
 	write_unlock(&act_mod_lock);
 	return 0;
@@ -1086,12 +1089,6 @@ tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	memset(&a, 0, sizeof(struct tc_action));
 	a.ops = a_o;
 
-	if (a_o->walk == NULL) {
-		WARN(1, "tc_dump_action: %s !capable of dumping table\n",
-		     a_o->kind);
-		goto out_module_put;
-	}
-
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
-- 
1.7.10.4

[PATCH 4/5] net_sched: Use default action walker methods

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_csum.c    |    1 -
 net/sched/act_gact.c    |    1 -
 net/sched/act_ipt.c     |    2 --
 net/sched/act_mirred.c  |    1 -
 net/sched/act_nat.c     |    1 -
 net/sched/act_pedit.c   |    1 -
 net/sched/act_simple.c  |    1 -
 net/sched/act_skbedit.c |    1 -
 8 files changed, 9 deletions(-)

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 4225a93..5c5edf5 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -586,7 +586,6 @@ static struct tc_action_ops act_csum_ops = {
 	.dump		= tcf_csum_dump,
 	.cleanup	= tcf_csum_cleanup,
 	.init		= tcf_csum_init,
-	.walk		= tcf_generic_walker
 };
 
 MODULE_DESCRIPTION("Checksum updating actions");
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 15851da..5645a4d 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -207,7 +207,6 @@ static struct tc_action_ops act_gact_ops = {
 	.dump		=	tcf_gact_dump,
 	.cleanup	=	tcf_gact_cleanup,
 	.init		=	tcf_gact_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002-4)");
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 1d3e191..882a897 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -299,7 +299,6 @@ static struct tc_action_ops act_ipt_ops = {
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
 	.init		=	tcf_ipt_init,
-	.walk		=	tcf_generic_walker
 };
 
 static struct tc_action_ops act_xt_ops = {
@@ -312,7 +311,6 @@ static struct tc_action_ops act_xt_ops = {
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
 	.init		=	tcf_ipt_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002-13)");
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 6cb16ec..2523781 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -272,7 +272,6 @@ static struct tc_action_ops act_mirred_ops = {
 	.dump		=	tcf_mirred_dump,
 	.cleanup	=	tcf_mirred_cleanup,
 	.init		=	tcf_mirred_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002)");
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 30c13de..6a15ace 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -309,7 +309,6 @@ static struct tc_action_ops act_nat_ops = {
 	.dump		=	tcf_nat_dump,
 	.cleanup	=	tcf_nat_cleanup,
 	.init		=	tcf_nat_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_DESCRIPTION("Stateless NAT actions");
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index ab4fc56..03b6767 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -244,7 +244,6 @@ static struct tc_action_ops act_pedit_ops = {
 	.dump		=	tcf_pedit_dump,
 	.cleanup	=	tcf_pedit_cleanup,
 	.init		=	tcf_pedit_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002-4)");
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 7725eb4..31157d3 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -201,7 +201,6 @@ static struct tc_action_ops act_simp_ops = {
 	.dump		=	tcf_simp_dump,
 	.cleanup	=	tcf_simp_cleanup,
 	.init		=	tcf_simp_init,
-	.walk		=	tcf_generic_walker,
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2005)");
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index cb42211..35ea643 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -203,7 +203,6 @@ static struct tc_action_ops act_skbedit_ops = {
 	.dump		=	tcf_skbedit_dump,
 	.cleanup	=	tcf_skbedit_cleanup,
 	.init		=	tcf_skbedit_init,
-	.walk		=	tcf_generic_walker,
 };
 
 MODULE_AUTHOR("Alexander Duyck, <alexander.h.duyck@intel.com>");
-- 
1.7.10.4

[PATCH 5/5] net_sched: Fail if missing mandatory action operation methods

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 401eca8..c708f3b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -277,8 +277,10 @@ int tcf_register_action(struct tc_action_ops *act)
 			return -EEXIST;
 		}
 	}
-	act->next = NULL;
-	*ap = act;
+
+	/* Must supply act, dump, cleanup and init */
+	if (!act->act || !act->dump || !act->cleanup || !act->init)
+		return -EINVAL;
 
 	/* Supply defaults */
 	if (!act->lookup)
@@ -286,6 +288,8 @@ int tcf_register_action(struct tc_action_ops *act)
 	if (!act->walk)
 		act->walk = tcf_generic_walker;
 
+	act->next = NULL;
+	*ap = act;
 	write_unlock(&act_mod_lock);
 	return 0;
 }
@@ -388,7 +392,7 @@ int tcf_action_exec(struct sk_buff *skb, const struct tc_action *act,
 	}
 	while ((a = act) != NULL) {
 repeat:
-		if (a->ops && a->ops->act) {
+		if (a->ops) {
 			ret = a->ops->act(skb, a, res);
 			if (TC_MUNGED & skb->tc_verd) {
 				/* copied already, allow trampling */
@@ -412,7 +416,7 @@ void tcf_action_destroy(struct tc_action *act, int bind)
 	struct tc_action *a;
 
 	for (a = act; a; a = act) {
-		if (a->ops && a->ops->cleanup) {
+		if (a->ops) {
 			if (a->ops->cleanup(a, bind) == ACT_P_DELETED)
 				module_put(a->ops->owner);
 			act = act->next;
@@ -431,7 +435,7 @@ tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 {
 	int err = -EINVAL;
 
-	if (a->ops == NULL || a->ops->dump == NULL)
+	if (a->ops == NULL)
 		return err;
 	return a->ops->dump(skb, a, bind, ref);
 }
@@ -443,7 +447,7 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
-	if (a->ops == NULL || a->ops->dump == NULL)
+	if (a->ops == NULL)
 		return err;
 
 	if (nla_put_string(skb, TCA_KIND, a->ops->kind))
-- 
1.7.10.4

Re: [PATCH 5/5] net_sched: Fail if missing mandatory action operation methods

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Tue,  3 Dec 2013 09:23:19 -0500

> @@ -277,8 +277,10 @@ int tcf_register_action(struct tc_action_ops *act)
>  			return -EEXIST;
>  		}
>  	}
> -	act->next = NULL;
> -	*ap = act;
> +
> +	/* Must supply act, dump, cleanup and init */
> +	if (!act->act || !act->dump || !act->cleanup || !act->init)
> +		return -EINVAL;
>  
>  	/* Supply defaults */
>  	if (!act->lookup)

You are going to return with a lock held on error here.

I was going to tell you not to do these fixups so late in the
function, inside of the write lock, there is no reason for it.

You should do all of this operation handling at the top of
the function, before the write lock is taken.

Re: [PATCH 5/5] net_sched: Fail if missing mandatory action operation methods

[Jamal Hadi Salim]

On 12/03/13 11:48, David Miller wrote:

> You are going to return with a lock held on error here.
>
> I was going to tell you not to do these fixups so late in the
> function, inside of the write lock, there is no reason for it.
>
> You should do all of this operation handling at the top of
> the function, before the write lock is taken.
>

yikes;->
do you want the whole set resent or the last one only?

cheers,
jamal

Re: [PATCH 5/5] net_sched: Fail if missing mandatory action operation methods

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Tue, 03 Dec 2013 19:43:20 -0500

> On 12/03/13 11:48, David Miller wrote:
> 
>> You are going to return with a lock held on error here.
>>
>> I was going to tell you not to do these fixups so late in the
>> function, inside of the write lock, there is no reason for it.
>>
>> You should do all of this operation handling at the top of
>> the function, before the write lock is taken.
>>
> 
> yikes;->
> do you want the whole set resent or the last one only?

Whole set please.

Re: [PATCH 5/5] net_sched: Fail if missing mandatory action operation methods

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Tue, 03 Dec 2013 19:43:20 -0500

> On 12/03/13 11:48, David Miller wrote:
> 
>> You are going to return with a lock held on error here.
>>
>> I was going to tell you not to do these fixups so late in the
>> function, inside of the write lock, there is no reason for it.
>>
>> You should do all of this operation handling at the top of
>> the function, before the write lock is taken.
>>
> 
> yikes;->
> do you want the whole set resent or the last one only?

BTW, you'll have to resend the whole set because your initial
patches put the !act->$(OP) checks inside the lock, and I'm
telling you here to put it outside the lock.

In fact, 3 of your patches need to change because of this.