* [PATCH 1/4] Provide default lookup function for actions that dont provide one
2013-12-01 22:30 [PATCH 0/4] net_sched: actions - Add default lookup and walker Jamal Hadi Salim
@ 2013-12-01 22:30 ` Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 2/4] Use default lookup functions. Users are free to override when needed Jamal Hadi Salim
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2013-12-01 22:30 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, alexander.h.duyck, ebiederm, jhs
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu•com>
---
net/sched/act_api.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index fd70728..3c974a0 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;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/4] Use default lookup functions. Users are free to override when needed.
2013-12-01 22:30 [PATCH 0/4] net_sched: actions - Add default lookup and walker Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 1/4] Provide default lookup function for actions that dont provide one Jamal Hadi Salim
@ 2013-12-01 22:30 ` Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 3/4] Provide default walker function for actions that dont provide one Jamal Hadi Salim
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2013-12-01 22:30 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, alexander.h.duyck, ebiederm, jhs
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] Provide default walker function for actions that dont provide one
2013-12-01 22:30 [PATCH 0/4] net_sched: actions - Add default lookup and walker Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 1/4] Provide default lookup function for actions that dont provide one Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 2/4] Use default lookup functions. Users are free to override when needed Jamal Hadi Salim
@ 2013-12-01 22:30 ` Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 4/4] Use default walker functions. Users who need something special can override Jamal Hadi Salim
2013-12-02 21:27 ` [PATCH 0/4] net_sched: actions - Add default lookup and walker David Miller
4 siblings, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2013-12-01 22:30 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, alexander.h.duyck, ebiederm, jhs
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu•com>
---
net/sched/act_api.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3c974a0..d275482 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;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 4/4] Use default walker functions. Users who need something special can override.
2013-12-01 22:30 [PATCH 0/4] net_sched: actions - Add default lookup and walker Jamal Hadi Salim
` (2 preceding siblings ...)
2013-12-01 22:30 ` [PATCH 3/4] Provide default walker function for actions that dont provide one Jamal Hadi Salim
@ 2013-12-01 22:30 ` Jamal Hadi Salim
2013-12-02 21:27 ` [PATCH 0/4] net_sched: actions - Add default lookup and walker David Miller
4 siblings, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2013-12-01 22:30 UTC (permalink / raw)
To: davem; +Cc: netdev, eric.dumazet, alexander.h.duyck, ebiederm, jhs
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 0/4] net_sched: actions - Add default lookup and walker
2013-12-01 22:30 [PATCH 0/4] net_sched: actions - Add default lookup and walker Jamal Hadi Salim
` (3 preceding siblings ...)
2013-12-01 22:30 ` [PATCH 4/4] Use default walker functions. Users who need something special can override Jamal Hadi Salim
@ 2013-12-02 21:27 ` David Miller
2013-12-03 13:34 ` Jamal Hadi Salim
4 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2013-12-02 21:27 UTC (permalink / raw)
To: jhs; +Cc: netdev, eric.dumazet, alexander.h.duyck, ebiederm
From: Jamal Hadi Salim <jhs@mojatatu•com>
Date: Sun, 1 Dec 2013 17:30:05 -0500
> This set of patches provide defaults for lookup and walkers for actions.
> Users can override when needed.
>
> Jamal Hadi Salim (4):
> Provide default lookup function for actions that dont provide one
> Use default lookup functions. Users are free to override when needed.
> Provide default walker function for actions that dont provide one
> Use default walker functions. Users who need something special can
> override.
Ok, but can you respin this set, adding in changes to remove
the code that checks for NULL in these operations?
Specifically, we still have:
if (a->ops->lookup == NULL)
goto err_mod;
in tcf_action_get_1(). And:
if (a_o->walk == NULL) {
in tc_dump_action().
Furthermore, there is a hard assumption that a_o->init() is non-NULL
as well. So maybe add a:
if (!act->init)
return -EINVAL;
at the top of tcf_register_action().
It also occurred to me that it would be simpler to audit and be a
simpler patch set to review if you just added the missing ".lookup ="
assignments to the actions and added a similar "if (!act->lookup)" et
al. check to tcf_register_action().
That looks more like a rigorous requirement to have a valid method
present for these three operations: init, lookup, walk.
What do you think?
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 0/4] net_sched: actions - Add default lookup and walker
2013-12-02 21:27 ` [PATCH 0/4] net_sched: actions - Add default lookup and walker David Miller
@ 2013-12-03 13:34 ` Jamal Hadi Salim
0 siblings, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2013-12-03 13:34 UTC (permalink / raw)
To: David Miller; +Cc: netdev, eric.dumazet, alexander.h.duyck, ebiederm
On 12/02/13 16:27, David Miller wrote:
>
> Ok, but can you respin this set, adding in changes to remove
> the code that checks for NULL in these operations?
>
> Specifically, we still have:
>
> if (a->ops->lookup == NULL)
> goto err_mod;
>
> in tcf_action_get_1(). And:
>
> if (a_o->walk == NULL) {
>
> in tc_dump_action().
>
Ok, I think i finaly understood what you were saying earlier;->
I was too focused on the simple fix needed for Eric B.
Will make this change ..
> Furthermore, there is a hard assumption that a_o->init() is non-NULL
> as well. So maybe add a:
>
> if (!act->init)
> return -EINVAL;
>
> at the top of tcf_register_action().
>
Makes sense.
> It also occurred to me that it would be simpler to audit and be a
> simpler patch set to review if you just added the missing ".lookup ="
> assignments to the actions
Those two methods have proven to be always the defaults and only the
oddball action deviates. So my preference is to have defaults as per
that earlier patch (and overrides for the oddball).
>and added a similar "if (!act->lookup)" et
> al. check to tcf_register_action().
>
> That looks more like a rigorous requirement to have a valid method
> present for these three operations: init, lookup, walk.
>
> What do you think?
Yep - makes sense (I think you meant act, dump, cleanup and init)
I will add an extra patch..
cheers,
jamal
> Thanks!
>
^ permalink raw reply [flat|nested] 7+ messages in thread