[Patch net-next] net_sched: only create filter chains for new filters/actions

[Patch net-next] net_sched: only create filter chains for new filters/actions

[Cong Wang]

tcf_chain_get() always creates a new filter chain if not found
in existing ones. This is totally unnecessary when we get or
delete filters, new chain should be only created for new filters
(or new actions).

Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/pkt_cls.h |  3 ++-
 net/sched/act_api.c   |  2 +-
 net/sched/cls_api.c   | 13 +++++++++----
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 2c213a6..f776229 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -18,7 +18,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops);
 int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
 
 #ifdef CONFIG_NET_CLS
-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index);
+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
+				bool create);
 void tcf_chain_put(struct tcf_chain *chain);
 int tcf_block_get(struct tcf_block **p_block,
 		  struct tcf_proto __rcu **p_filter_chain);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 0ecf2a8..aed6cf2 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -34,7 +34,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
 
 	if (!tp)
 		return -EINVAL;
-	a->goto_chain = tcf_chain_get(tp->chain->block, chain_index);
+	a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
 	if (!a->goto_chain)
 		return -ENOMEM;
 	return 0;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 01a8b8b..23d2236 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -220,7 +220,8 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
 	kfree(chain);
 }
 
-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
+				bool create)
 {
 	struct tcf_chain *chain;
 
@@ -230,7 +231,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
 			return chain;
 		}
 	}
-	return tcf_chain_create(block, chain_index);
+	if (create)
+		return tcf_chain_create(block, chain_index);
+	else
+		return NULL;
 }
 EXPORT_SYMBOL(tcf_chain_get);
 
@@ -509,9 +513,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 		err = -EINVAL;
 		goto errout;
 	}
-	chain = tcf_chain_get(block, chain_index);
+	chain = tcf_chain_get(block, chain_index,
+			      n->nlmsg_type == RTM_NEWTFILTER);
 	if (!chain) {
-		err = -ENOMEM;
+		err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
 		goto errout;
 	}
 
-- 
2.5.5

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Jiri Pirko]

Tue, May 23, 2017 at 06:42:37PM CEST, xiyou.wangcong@gmail.com wrote:
>tcf_chain_get() always creates a new filter chain if not found
>in existing ones. This is totally unnecessary when we get or
>delete filters, new chain should be only created for new filters
>(or new actions).
>
>Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>---
> include/net/pkt_cls.h |  3 ++-
> net/sched/act_api.c   |  2 +-
> net/sched/cls_api.c   | 13 +++++++++----
> 3 files changed, 12 insertions(+), 6 deletions(-)
>
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 2c213a6..f776229 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -18,7 +18,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops);
> int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
> 
> #ifdef CONFIG_NET_CLS
>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index);
>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>+				bool create);
> void tcf_chain_put(struct tcf_chain *chain);
> int tcf_block_get(struct tcf_block **p_block,
> 		  struct tcf_proto __rcu **p_filter_chain);
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 0ecf2a8..aed6cf2 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -34,7 +34,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
> 
> 	if (!tp)
> 		return -EINVAL;
>-	a->goto_chain = tcf_chain_get(tp->chain->block, chain_index);
>+	a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
> 	if (!a->goto_chain)
> 		return -ENOMEM;
> 	return 0;
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index 01a8b8b..23d2236 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -220,7 +220,8 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
> 	kfree(chain);
> }
> 
>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>+				bool create)
> {
> 	struct tcf_chain *chain;
> 
>@@ -230,7 +231,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
> 			return chain;
> 		}
> 	}
>-	return tcf_chain_create(block, chain_index);
>+	if (create)
>+		return tcf_chain_create(block, chain_index);
>+	else
>+		return NULL;
> }
> EXPORT_SYMBOL(tcf_chain_get);
> 
>@@ -509,9 +513,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
> 		err = -EINVAL;
> 		goto errout;
> 	}
>-	chain = tcf_chain_get(block, chain_index);
>+	chain = tcf_chain_get(block, chain_index,
>+			      n->nlmsg_type == RTM_NEWTFILTER);

First of all, I really hate all these true/false arg dances. Totaly
confusing all the time.



> 	if (!chain) {
>-		err = -ENOMEM;
>+		err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;

Confusing. Please do not obfuscate the code for a corner cases. Thanks.



> 		goto errout;
> 	}
> 
>-- 
>2.5.5
>

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Cong Wang]

On Tue, May 23, 2017 at 11:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, May 23, 2017 at 06:42:37PM CEST, xiyou.wangcong@gmail.com wrote:
>>tcf_chain_get() always creates a new filter chain if not found
>>in existing ones. This is totally unnecessary when we get or
>>delete filters, new chain should be only created for new filters
>>(or new actions).
>>
>>Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>>Cc: Jiri Pirko <jiri@mellanox.com>
>>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>---
>> include/net/pkt_cls.h |  3 ++-
>> net/sched/act_api.c   |  2 +-
>> net/sched/cls_api.c   | 13 +++++++++----
>> 3 files changed, 12 insertions(+), 6 deletions(-)
>>
>>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>index 2c213a6..f776229 100644
>>--- a/include/net/pkt_cls.h
>>+++ b/include/net/pkt_cls.h
>>@@ -18,7 +18,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops);
>> int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
>>
>> #ifdef CONFIG_NET_CLS
>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index);
>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>+                              bool create);
>> void tcf_chain_put(struct tcf_chain *chain);
>> int tcf_block_get(struct tcf_block **p_block,
>>                 struct tcf_proto __rcu **p_filter_chain);
>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>index 0ecf2a8..aed6cf2 100644
>>--- a/net/sched/act_api.c
>>+++ b/net/sched/act_api.c
>>@@ -34,7 +34,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
>>
>>       if (!tp)
>>               return -EINVAL;
>>-      a->goto_chain = tcf_chain_get(tp->chain->block, chain_index);
>>+      a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
>>       if (!a->goto_chain)
>>               return -ENOMEM;
>>       return 0;
>>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>index 01a8b8b..23d2236 100644
>>--- a/net/sched/cls_api.c
>>+++ b/net/sched/cls_api.c
>>@@ -220,7 +220,8 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
>>       kfree(chain);
>> }
>>
>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>+                              bool create)
>> {
>>       struct tcf_chain *chain;
>>
>>@@ -230,7 +231,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>                       return chain;
>>               }
>>       }
>>-      return tcf_chain_create(block, chain_index);
>>+      if (create)
>>+              return tcf_chain_create(block, chain_index);
>>+      else
>>+              return NULL;
>> }
>> EXPORT_SYMBOL(tcf_chain_get);
>>
>>@@ -509,9 +513,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>               err = -EINVAL;
>>               goto errout;
>>       }
>>-      chain = tcf_chain_get(block, chain_index);
>>+      chain = tcf_chain_get(block, chain_index,
>>+                            n->nlmsg_type == RTM_NEWTFILTER);
>
> First of all, I really hate all these true/false arg dances. Totaly
> confusing all the time.

Sounds like you are able to understand the code at all.
Sigh, I bet you never even read the changelog. ;)


>
>
>
>>       if (!chain) {
>>-              err = -ENOMEM;
>>+              err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
>
> Confusing. Please do not obfuscate the code for a corner cases. Thanks.

Either you don't understand the changelog or you don't understand
ternary conditional operator. I can't help you if the latter.

Sorry.

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Jiri Pirko]

Wed, May 24, 2017 at 05:53:42PM CEST, xiyou.wangcong@gmail.com wrote:
>On Tue, May 23, 2017 at 11:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, May 23, 2017 at 06:42:37PM CEST, xiyou.wangcong@gmail.com wrote:
>>>tcf_chain_get() always creates a new filter chain if not found
>>>in existing ones. This is totally unnecessary when we get or
>>>delete filters, new chain should be only created for new filters
>>>(or new actions).
>>>
>>>Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>>>Cc: Jiri Pirko <jiri@mellanox.com>
>>>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>>---
>>> include/net/pkt_cls.h |  3 ++-
>>> net/sched/act_api.c   |  2 +-
>>> net/sched/cls_api.c   | 13 +++++++++----
>>> 3 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>>index 2c213a6..f776229 100644
>>>--- a/include/net/pkt_cls.h
>>>+++ b/include/net/pkt_cls.h
>>>@@ -18,7 +18,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops);
>>> int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
>>>
>>> #ifdef CONFIG_NET_CLS
>>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index);
>>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>>+                              bool create);
>>> void tcf_chain_put(struct tcf_chain *chain);
>>> int tcf_block_get(struct tcf_block **p_block,
>>>                 struct tcf_proto __rcu **p_filter_chain);
>>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>>index 0ecf2a8..aed6cf2 100644
>>>--- a/net/sched/act_api.c
>>>+++ b/net/sched/act_api.c
>>>@@ -34,7 +34,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
>>>
>>>       if (!tp)
>>>               return -EINVAL;
>>>-      a->goto_chain = tcf_chain_get(tp->chain->block, chain_index);
>>>+      a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
>>>       if (!a->goto_chain)
>>>               return -ENOMEM;
>>>       return 0;
>>>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>>index 01a8b8b..23d2236 100644
>>>--- a/net/sched/cls_api.c
>>>+++ b/net/sched/cls_api.c
>>>@@ -220,7 +220,8 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
>>>       kfree(chain);
>>> }
>>>
>>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>>+                              bool create)
>>> {
>>>       struct tcf_chain *chain;
>>>
>>>@@ -230,7 +231,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>>                       return chain;
>>>               }
>>>       }
>>>-      return tcf_chain_create(block, chain_index);
>>>+      if (create)
>>>+              return tcf_chain_create(block, chain_index);
>>>+      else
>>>+              return NULL;
>>> }
>>> EXPORT_SYMBOL(tcf_chain_get);
>>>
>>>@@ -509,9 +513,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>>               err = -EINVAL;
>>>               goto errout;
>>>       }
>>>-      chain = tcf_chain_get(block, chain_index);
>>>+      chain = tcf_chain_get(block, chain_index,
>>>+                            n->nlmsg_type == RTM_NEWTFILTER);
>>
>> First of all, I really hate all these true/false arg dances. Totaly
>> confusing all the time.
>
>Sounds like you are able to understand the code at all.
>Sigh, I bet you never even read the changelog. ;)
>
>
>>
>>
>>
>>>       if (!chain) {
>>>-              err = -ENOMEM;
>>>+              err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
>>
>> Confusing. Please do not obfuscate the code for a corner cases. Thanks.
>
>Either you don't understand the changelog or you don't understand
>ternary conditional operator. I can't help you if the latter.
>
>Sorry.

Heh. Really? All I say is, your patch is not needed at all. All it adds
makes to code harder to understand and no benefit.

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 23 May 2017 09:42:37 -0700

> tcf_chain_get() always creates a new filter chain if not found
> in existing ones. This is totally unnecessary when we get or
> delete filters, new chain should be only created for new filters
> (or new actions).
> 
> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Indeed, get and delete requests should not create new objects, ever.

I have pretty much no idea why Jiri is making such a big fuss about
this change, to be quite honest. :-)

Applied, thanks Cong.

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Jiri Pirko]

Thu, May 25, 2017 at 06:14:56PM CEST, davem@davemloft.net wrote:
>From: Cong Wang <xiyou.wangcong@gmail.com>
>Date: Tue, 23 May 2017 09:42:37 -0700
>
>> tcf_chain_get() always creates a new filter chain if not found
>> in existing ones. This is totally unnecessary when we get or
>> delete filters, new chain should be only created for new filters
>> (or new actions).
>> 
>> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>> Cc: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
>Indeed, get and delete requests should not create new objects, ever.
>
>I have pretty much no idea why Jiri is making such a big fuss about
>this change, to be quite honest. :-)

Because it makes already hard to read code even worse, for *no* benefit.
That's why.


>
>Applied, thanks Cong.

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 26 May 2017 07:53:52 +0200

> Thu, May 25, 2017 at 06:14:56PM CEST, davem@davemloft.net wrote:
>>From: Cong Wang <xiyou.wangcong@gmail.com>
>>Date: Tue, 23 May 2017 09:42:37 -0700
>>
>>> tcf_chain_get() always creates a new filter chain if not found
>>> in existing ones. This is totally unnecessary when we get or
>>> delete filters, new chain should be only created for new filters
>>> (or new actions).
>>> 
>>> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>>> Cc: Jiri Pirko <jiri@mellanox.com>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>
>>Indeed, get and delete requests should not create new objects, ever.
>>
>>I have pretty much no idea why Jiri is making such a big fuss about
>>this change, to be quite honest. :-)
> 
> Because it makes already hard to read code even worse, for *no* benefit.
> That's why.

Jiri, if you say the same thing 100 times, it doesn't help anyone
understand your arguments any better.

Creating new objects when a GET or a DEL is requested is flat out
wrong.

And Cong is fixing that.

And I also didn't find the boolean logic hard to understand at all.

It is in fact a very common pattern to pass a "create" boolean into
lookup functions, to tell them whether to create a new object on
lookup failure or not.  And then also to control that boolean via
what kind of netlink request we are processing.

So you tell me what's so bad about his change given the above?

Give me details and real facts, like I just did, rather than vague
statements about "benefit" and "hard to read".

Thank you.

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Cong Wang]

On Fri, May 26, 2017 at 7:54 AM, David Miller <davem@davemloft.net> wrote:
> And I also didn't find the boolean logic hard to understand at all.
>
> It is in fact a very common pattern to pass a "create" boolean into
> lookup functions, to tell them whether to create a new object on
> lookup failure or not.  And then also to control that boolean via
> what kind of netlink request we are processing.

+10

It is a widely used pattern among the kernel source code.
I'd be surprised if an experienced kernel developer is not
aware of this pattern. ;)

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Jiri Pirko]

Fri, May 26, 2017 at 06:55:25PM CEST, xiyou.wangcong@gmail.com wrote:
>On Fri, May 26, 2017 at 7:54 AM, David Miller <davem@davemloft.net> wrote:
>> And I also didn't find the boolean logic hard to understand at all.
>>
>> It is in fact a very common pattern to pass a "create" boolean into
>> lookup functions, to tell them whether to create a new object on
>> lookup failure or not.  And then also to control that boolean via
>> what kind of netlink request we are processing.
>
>+10
>
>It is a widely used pattern among the kernel source code.
>I'd be surprised if an experienced kernel developer is not
>aware of this pattern. ;)

Cong, as you wisely put, I'm not aware of this pattern and I'm also
unaware of existence of ternary operator. Are this notes necessary?
Does that make you feel better?

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Cong Wang]

On Sat, May 27, 2017 at 3:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Cong, as you wisely put, I'm not aware of this pattern and I'm also
> unaware of existence of ternary operator. Are this notes necessary?
> Does that make you feel better?
>

You are more than just welcome to redirect my email to
your /dev/null. I am quite sure my email is waste for you.

Thanks!!

Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Jiri Pirko]

Fri, May 26, 2017 at 04:54:43PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Fri, 26 May 2017 07:53:52 +0200
>
>> Thu, May 25, 2017 at 06:14:56PM CEST, davem@davemloft.net wrote:
>>>From: Cong Wang <xiyou.wangcong@gmail.com>
>>>Date: Tue, 23 May 2017 09:42:37 -0700
>>>
>>>> tcf_chain_get() always creates a new filter chain if not found
>>>> in existing ones. This is totally unnecessary when we get or
>>>> delete filters, new chain should be only created for new filters
>>>> (or new actions).
>>>> 
>>>> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>>>> Cc: Jiri Pirko <jiri@mellanox.com>
>>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>>
>>>Indeed, get and delete requests should not create new objects, ever.
>>>
>>>I have pretty much no idea why Jiri is making such a big fuss about
>>>this change, to be quite honest. :-)
>> 
>> Because it makes already hard to read code even worse, for *no* benefit.
>> That's why.
>
>Jiri, if you say the same thing 100 times, it doesn't help anyone
>understand your arguments any better.
>
>Creating new objects when a GET or a DEL is requested is flat out
>wrong.
 
Allright. I ack that.


>
>And Cong is fixing that.
>
>And I also didn't find the boolean logic hard to understand at all.
>
>It is in fact a very common pattern to pass a "create" boolean into
>lookup functions, to tell them whether to create a new object on
>lookup failure or not.  And then also to control that boolean via
>what kind of netlink request we are processing.
>
>So you tell me what's so bad about his change given the above?
>
>Give me details and real facts, like I just did, rather than vague
>statements about "benefit" and "hard to read".

What I don't like is the double "n->nlmsg_type == RTM_NEWTFILTER" check
and return value decusion according to the latter check. The code logic
is split into tcf_chain_get function and its caller. That is
at least odd.

Since you don't like the PTR_ERR approach, I'll try to figure out how to
do this another way.