| Message ID | 20170526072129.11075-1-jiri@resnulli.us |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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.
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; }