[patch net-next] net/sched: let chain_get to figure out the return value

[patch net-next] net/sched: let chain_get to figure out the return value

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Alhough I believe that this create/nocreate dance is completelly
pointless, at least make it a bit nicer and easier to read.
Push the decision on what error value is returned to chain_get function
and use ERR macros.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/act_api.c |  4 ++--
 net/sched/cls_api.c | 15 ++++++---------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index aed6cf2..42bb75a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -35,8 +35,8 @@ 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, true);
-	if (!a->goto_chain)
-		return -ENOMEM;
+	if (IS_ERR(a->goto_chain))
+		return PTR_ERR(a->goto_chain);
 	return 0;
 }
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 39da0c5..deec814 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -193,7 +193,7 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
 
 	chain = kzalloc(sizeof(*chain), GFP_KERNEL);
 	if (!chain)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	list_add_tail(&chain->list, &block->chain_list);
 	chain->block = block;
 	chain->index = chain_index;
@@ -231,10 +231,7 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
 			return chain;
 		}
 	}
-	if (create)
-		return tcf_chain_create(block, chain_index);
-	else
-		return NULL;
+	return create ? tcf_chain_create(block, chain_index) : ERR_PTR(-EINVAL);
 }
 EXPORT_SYMBOL(tcf_chain_get);
 
@@ -267,8 +264,8 @@ int tcf_block_get(struct tcf_block **p_block,
 	INIT_LIST_HEAD(&block->chain_list);
 	/* Create chain 0 by default, it has to be always present. */
 	chain = tcf_chain_create(block, 0);
-	if (!chain) {
-		err = -ENOMEM;
+	if (IS_ERR(chain)) {
+		err = PTR_ERR(chain);
 		goto err_chain_create;
 	}
 	tcf_chain_filter_chain_ptr_set(chain, p_filter_chain);
@@ -517,8 +514,8 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 	}
 	chain = tcf_chain_get(block, chain_index,
 			      n->nlmsg_type == RTM_NEWTFILTER);
-	if (!chain) {
-		err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
+	if (IS_ERR(chain)) {
+		err = PTR_ERR(chain);
 		goto errout;
 	}
 
-- 
2.9.3

Re: [patch net-next] net/sched: let chain_get to figure out the return value

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 26 May 2017 09:21:29 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Alhough I believe that this create/nocreate dance is completelly
> pointless, at least make it a bit nicer and easier to read.
> Push the decision on what error value is returned to chain_get function
> and use ERR macros.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

No, this is quite worse.

You're leaving pointer error values in structures.  That's extremely
error prone.

And as stated in the other thread, I don't think Cong's logic is strange
or hard to understand at all.

Re: [patch net-next] net/sched: let chain_get to figure out the return value

[Jiri Pirko]

Fri, May 26, 2017 at 04:59:12PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Fri, 26 May 2017 09:21:29 +0200
>
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Alhough I believe that this create/nocreate dance is completelly
>> pointless, at least make it a bit nicer and easier to read.
>> Push the decision on what error value is returned to chain_get function
>> and use ERR macros.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>No, this is quite worse.
>
>You're leaving pointer error values in structures.  That's extremely
>error prone.

Yet used everywhere in kernel.

>
>And as stated in the other thread, I don't think Cong's logic is strange
>or hard to understand at all.

That is why tc code looks how it does :/
But perhaps I'm slow and everything is crystal-clear to everyone else.