| Message ID | 53287010.8000703@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Behalf Of Balakumaran Kannan > Remove unnecessary assignments > > Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com> > --- > 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: > - 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-03-18 at 16:22 +0000, David Laight wrote: > From: Behalf Of Balakumaran Kannan > > Remove unnecessary assignments > > > > Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com> > > --- > > 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: > > - 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. 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) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks David and Joe for clarifications Regards, K.Balakumaran On Tue, Mar 18, 2014 at 10:33 PM, Joe Perches <joe@perches.com> wrote: > On Tue, 2014-03-18 at 16:22 +0000, David Laight wrote: >> From: Behalf Of Balakumaran Kannan >> > Remove unnecessary assignments >> > >> > Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com> >> > --- >> > 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: >> > - 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. > > 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) > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- net/ipv4/devinet.c.orig 2014-03-19 02:15:33.000000000 +0530 +++ net/ipv4/devinet.c 2014-03-19 03:02:16.000000000 +0530 @@ -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();
Remove unnecessary assignments Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com> --- 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. --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html