[PATCH net-next v3] net: assign err to 0 at begin in do_setlink() function

[PATCH net-next v3] net: assign err to 0 at begin in do_setlink() function

[yuan linyu]

From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>

each netlink attribute have proper process when error happen,
when exit one attribute process, it implies that no error,
so err = 0; is useless.

assign err = 0; at beginning if all attributes not set.

v1 -> v2:
	fix review comment from David, clear err before
	nla_for_each_nested()

v2 -> v3:
	maybe wrong understanding of David comment,
	provide a new version

Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
 net/core/rtnetlink.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index dabba2a..54f792b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2075,7 +2075,7 @@ static int do_setlink(const struct sk_buff *skb,
 		      struct nlattr **tb, char *ifname, int status)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
-	int err;
+	int err = 0;
 
 	if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]) {
 		struct net *net = rtnl_link_get_net(dev_net(dev), tb);
@@ -2253,7 +2253,6 @@ static int do_setlink(const struct sk_buff *skb,
 			status |= DO_SETLINK_NOTIFY;
 		}
 	}
-	err = 0;
 
 	if (tb[IFLA_VF_PORTS]) {
 		struct nlattr *port[IFLA_PORT_MAX+1];
@@ -2261,9 +2260,10 @@ static int do_setlink(const struct sk_buff *skb,
 		int vf;
 		int rem;
 
-		err = -EOPNOTSUPP;
-		if (!ops->ndo_set_vf_port)
+		if (!ops->ndo_set_vf_port) {
+			err = -EOPNOTSUPP;
 			goto errout;
+		}
 
 		nla_for_each_nested(attr, tb[IFLA_VF_PORTS], rem) {
 			if (nla_type(attr) != IFLA_VF_PORT ||
@@ -2286,7 +2286,6 @@ static int do_setlink(const struct sk_buff *skb,
 			status |= DO_SETLINK_NOTIFY;
 		}
 	}
-	err = 0;
 
 	if (tb[IFLA_PORT_SELF]) {
 		struct nlattr *port[IFLA_PORT_MAX+1];
@@ -2326,7 +2325,6 @@ static int do_setlink(const struct sk_buff *skb,
 			status |= DO_SETLINK_NOTIFY;
 		}
 	}
-	err = 0;
 
 	if (tb[IFLA_PROTO_DOWN]) {
 		err = dev_change_proto_down(dev,
-- 
2.7.4

Re: [PATCH net-next v3] net: assign err to 0 at begin in do_setlink() function

[David Miller]

From: yuan linyu <cugyly@163.com>
Date: Thu, 16 Nov 2017 19:59:48 +0800

> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> 
> each netlink attribute have proper process when error happen,
> when exit one attribute process, it implies that no error,
> so err = 0; is useless.
> 
> assign err = 0; at beginning if all attributes not set.
> 
> v1 -> v2:
> 	fix review comment from David, clear err before
> 	nla_for_each_nested()
> 
> v2 -> v3:
> 	maybe wrong understanding of David comment,
> 	provide a new version
> 
> Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>

I'm sorry I still find it hard to accept this change.

What about all of the assignments of 'err' which only branch to
'errout' if err is negative?  It is not easy to see that none of those
case ever result in 'err' holding a positive non-zero value.

The code as-is is the easiest to understand, audit and prove correct
in the error-free case.  And this because of the explicit clearing or
'err' to zero late in the function.

Thanks you.

Re: [PATCH net-next v3] net: assign err to 0 at begin in do_setlink() function

[yuan linyu]

On 五, 2017-11-17 at 15:08 +0900, David Miller wrote:
> From: yuan linyu <cugyly@163.com>
> Date: Thu, 16 Nov 2017 19:59:48 +0800
> 
> > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> > 
> > each netlink attribute have proper process when error happen,
> > when exit one attribute process, it implies that no error,
> > so err = 0; is useless.
> > 
> > assign err = 0; at beginning if all attributes not set.
> > 
> > v1 -> v2:
> >       fix review comment from David, clear err before
> >       nla_for_each_nested()
> > 
> > v2 -> v3:
> >       maybe wrong understanding of David comment,
> >       provide a new version
> > 
> > Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> 
> I'm sorry I still find it hard to accept this change.
> 
> What about all of the assignments of 'err' which only branch to
> 'errout' if err is negative?  It is not easy to see that none of those
> case ever result in 'err' holding a positive non-zero value.
yes, i also try to check any function return > 0, but it hard,
maybe some function not follow common rule.
> 
> The code as-is is the easiest to understand, audit and prove correct
> in the error-free case.  And this because of the explicit clearing or
> 'err' to zero late in the function.
yes, but this clearing will do three times every function call,
but it is not big issue 
> 
> Thanks you.