Skip to contentSkip to conversation

net : Remove unnecessary assignments

PATCH In review Balakumaran Kannan 3 participants 4 messages Mar 18, 2014 – Mar 19, 2014

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Balakumaran Kannan

author
#0 lore raw

PATCH 1/1 net : Remove unnecessary assignments

Remove unnecessary assignments

Signed-off-by Balakumaran Kannan <[email protected]>

---
Some unnecessary assignments has been removed from devinet_ioctl function. The 'ret' variable is set in multiple places, but used only based on if conditions. So keeping the assignment inside if will be a better.

Patch is prepared for Linux-3.14-rc7 kernel.
---

ipv4/devinet.c renamed from ipv4/devinet.c.orig +42 -26
@@ -886,15 +886,17 @@ int devinet_ioctl(struct net *net, unsig
 	struct in_ifaddr *ifa = NULL;
 	struct net_device *dev;
 	char *colon;
-	int ret = -EFAULT;
+	int ret;
 	int tryaddrmatch = 0;
 
 	/*
 	 *	Fetch the caller's info block into kernel space
 	 */
 
-	if (copy_from_user(&ifr, arg, sizeof(struct ifreq)))
+	if (copy_from_user(&ifr, arg, sizeof(struct ifreq))) {
+		ret = -EFAULT;
 		goto out;
+	}
 	ifr.ifr_name[IFNAMSIZ - 1] = 0;
 
 	/* save original address for comparison */

@@ -921,20 +923,23 @@ int devinet_ioctl(struct net *net, unsig
 		break;
 
 	case SIOCSIFFLAGS:
-		ret = -EPERM;
-		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
+		if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) {
+			ret = -EPERM;
 			goto out;
+		}
 		break;
 	case SIOCSIFADDR:	/* Set interface address (and family) */
 	case SIOCSIFBRDADDR:	/* Set the broadcast address */
 	case SIOCSIFDSTADDR:	/* Set the destination address */
 	case SIOCSIFNETMASK: 	/* Set the netmask for the interface */
-		ret = -EPERM;
-		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
+		if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) {
+			ret = -EPERM;
 			goto out;
-		ret = -EINVAL;
-		if (sin->sin_family != AF_INET)
+		}
+		if (sin->sin_family != AF_INET) {
+			ret = -EINVAL;
 			goto out;
+		}
 		break;
 	default:
 		ret = -EINVAL;

@@ -943,10 +948,11 @@ int devinet_ioctl(struct net *net, unsig
 
 	rtnl_lock();
 
-	ret = -ENODEV;
 	dev = __dev_get_by_name(net, ifr.ifr_name);
-	if (!dev)
+	if (!dev) {
+		ret = -ENODEV;
 		goto done;
+	}
 
 	if (colon)
 		*colon = ':';

@@ -979,9 +985,10 @@ int devinet_ioctl(struct net *net, unsig
 		}
 	}
 
-	ret = -EADDRNOTAVAIL;
-	if (!ifa && cmd != SIOCSIFADDR && cmd != SIOCSIFFLAGS)
+	if (!ifa && cmd != SIOCSIFADDR && cmd != SIOCSIFFLAGS) {
+		ret = -EADDRNOTAVAIL;
 		goto done;
+	}
 
 	switch (cmd) {
 	case SIOCGIFADDR:	/* Get interface address */

@@ -1002,9 +1009,10 @@ int devinet_ioctl(struct net *net, unsig
 
 	case SIOCSIFFLAGS:
 		if (colon) {
-			ret = -EADDRNOTAVAIL;
-			if (!ifa)
+			if (!ifa) {
+				ret = -EADDRNOTAVAIL;
 				break;
+			}
 			ret = 0;
 			if (!(ifr.ifr_flags & IFF_UP))
 				inet_del_ifa(in_dev, ifap, 1);

@@ -1014,24 +1022,27 @@ int devinet_ioctl(struct net *net, unsig
 		break;
 
 	case SIOCSIFADDR:	/* Set interface address (and family) */
-		ret = -EINVAL;
-		if (inet_abc_len(sin->sin_addr.s_addr) < 0)
+		if (inet_abc_len(sin->sin_addr.s_addr) < 0) {
+			ret = -EINVAL;
 			break;
+		}
 
 		if (!ifa) {
-			ret = -ENOBUFS;
 			ifa = inet_alloc_ifa();
-			if (!ifa)
+			if (!ifa) {
+				ret = -ENOBUFS;
 				break;
+			}
 			INIT_HLIST_NODE(&ifa->hash);
 			if (colon)
 				memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
 			else
 				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
 		} else {
-			ret = 0;
-			if (ifa->ifa_local == sin->sin_addr.s_addr)
+			if (ifa->ifa_local == sin->sin_addr.s_addr) {
+				ret = 0;
 				break;
+			}
 			inet_del_ifa(in_dev, ifap, 0);
 			ifa->ifa_broadcast = 0;
 			ifa->ifa_scope = 0;

@@ -1064,12 +1075,14 @@ int devinet_ioctl(struct net *net, unsig
 		break;
 
 	case SIOCSIFDSTADDR:	/* Set the destination address */
-		ret = 0;
-		if (ifa->ifa_address == sin->sin_addr.s_addr)
+		if (ifa->ifa_address == sin->sin_addr.s_addr) {
+			ret = 0;
 			break;
-		ret = -EINVAL;
-		if (inet_abc_len(sin->sin_addr.s_addr) < 0)
+		}
+		if (inet_abc_len(sin->sin_addr.s_addr) < 0) {
+			ret = -EINVAL;
 			break;
+		}
 		ret = 0;
 		inet_del_ifa(in_dev, ifap, 0);
 		ifa->ifa_address = sin->sin_addr.s_addr;

@@ -1081,9 +1094,10 @@ int devinet_ioctl(struct net *net, unsig
 		/*
 		 *	The mask we set must be legal.
 		 */
-		ret = -EINVAL;
-		if (bad_mask(sin->sin_addr.s_addr, 0))
+		if (bad_mask(sin->sin_addr.s_addr, 0)) {
+			ret = -EINVAL;
 			break;
+		}
 		ret = 0;
 		if (ifa->ifa_mask != sin->sin_addr.s_addr) {
 			__be32 old_mask = ifa->ifa_mask;

@@ -1107,6 +1121,8 @@ int devinet_ioctl(struct net *net, unsig
 			inet_insert_ifa(ifa);
 		}
 		break;
+	default:
+		ret = -EADDRNOTAVAIL;
 	}
 done:
 	rtnl_unlock();
#1 lore raw

From: Behalf Of Balakumaran Kannan

Quoted 7 lines

Remove unnecessary assignments

Signed-off-by Balakumaran Kannan <[email protected]>

---
Some unnecessary assignments has been removed from devinet_ioctl function. The 'ret' variable is set in multiple places, but used only based on if conditions. So keeping the assignment inside if will be a better.

...

These all seem to be this transform:

ipv4/devinet.c int devinet_ioctl(struct net *net, unsig · line 955

Quoted 7 lines
ipv4/devinet.c renamed from ipv4/devinet.c.orig +3 -2
-	ret = -ENODEV;
 	dev = __dev_get_by_name(net, ifr.ifr_name);
-	if (!dev)
+	if (!dev) {
+		ret = -ENODEV;
 		goto done;
+	}

I bet that if you look at the generated code you'll find that the compiler generates the same code for both forms and is likely to generate the equivalent of:

	dev = __dev_get_by_name(net, ifr.ifr_name);
	ret = -ENODEV;
	if (!dev)
		goto done;

So this is just a matter of style.

David

#2 lore raw
Quoted 22 lines · David Laight

From: Behalf Of Balakumaran Kannan

Quoted 7 lines

Remove unnecessary assignments

Signed-off-by Balakumaran Kannan <[email protected]>

---
Some unnecessary assignments has been removed from devinet_ioctl function. The 'ret' variable is set in multiple places, but used only based on if conditions. So keeping the assignment inside if will be a better.

...

These all seem to be this transform:

ipv4/devinet.c int devinet_ioctl(struct net *net, unsig · line 955

Quoted 7 lines
ipv4/devinet.c renamed from ipv4/devinet.c.orig +3 -2
-	ret = -ENODEV;
 	dev = __dev_get_by_name(net, ifr.ifr_name);
-	if (!dev)
+	if (!dev) {
+		ret = -ENODEV;
 		goto done;
+	}

I bet that if you look at the generated code you'll find that the compiler generates the same code for both forms and is likely to generate the equivalent of:

Quoted 6 lines
	dev = __dev_get_by_name(net, ifr.ifr_name);
	ret = -ENODEV;
	if (!dev)
		goto done;

So this is just a matter of style.

Yabut it's Linus' preferred style.
http://lkml.org/lkml/2008/12/16/383

(but not mine, I prefer what's done by the patch)

author
#3 lore raw

Thanks David and Joe for clarifications

Regards,
K.Balakumaran

Quoted 36 lines · Joe Perches
Quoted 22 lines · David Laight

From: Behalf Of Balakumaran Kannan

Quoted 7 lines

Remove unnecessary assignments

Signed-off-by Balakumaran Kannan <[email protected]>

---
Some unnecessary assignments has been removed from devinet_ioctl function. The 'ret' variable is set in multiple places, but used only based on if conditions. So keeping the assignment inside if will be a better.

...

These all seem to be this transform:

ipv4/devinet.c int devinet_ioctl(struct net *net, unsig · line 955

Quoted 7 lines
ipv4/devinet.c renamed from ipv4/devinet.c.orig +3 -2
-   ret = -ENODEV;
    dev = __dev_get_by_name(net, ifr.ifr_name);
-   if (!dev)
+   if (!dev) {
+           ret = -ENODEV;
            goto done;
+   }

I bet that if you look at the generated code you'll find that the compiler generates the same code for both forms and is likely to generate the equivalent of:

Quoted 6 lines
      dev = __dev_get_by_name(net, ifr.ifr_name);
      ret = -ENODEV;
      if (!dev)
              goto done;

So this is just a matter of style.

Yabut it's Linus' preferred style.
http://lkml.org/lkml/2008/12/16/383

(but not mine, I prefer what's done by the patch)