2006-02-11 16:37:29

by Ingo Oeser

[permalink] [raw]
Subject: [PATCH] IPv6: Cleanups for net/ipv6/addrconf.c (kzalloc, early exit) v2

From: Ingo Oeser <[email protected]>

Here are some possible (and trivial) cleanups.
- use kzalloc() where possible
- remove unused label
- invert allocation failure test like
if (object) {
/* Rest of function here */
}
to

if (object == NULL)
return NULL;

/* Rest of function here */

The last one moves quite some code, because it changes indention.
I can split that up, if needed.

Signed-off-by: Ingo Oeser <[email protected]>
---
Hello,

On Thursday 09 February 2006 17:48, YOSHIFUJI Hideaki wrote:
> Please keep nlmsg_failure, which is used by NLMSG_NEW().

This issue has been adressed now.

Patch is against latest git from Linus. It has been compile tested with
allyesconfig on ix86.

Many thanks for your patience!

Regards

Ingo Oeer

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index b7d8822..984a9bc 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -327,86 +327,85 @@ static struct inet6_dev * ipv6_add_dev(s
if (dev->mtu < IPV6_MIN_MTU)
return NULL;

- ndev = kmalloc(sizeof(struct inet6_dev), GFP_KERNEL);
+ ndev = kzalloc(sizeof(struct inet6_dev), GFP_KERNEL);

- if (ndev) {
- memset(ndev, 0, sizeof(struct inet6_dev));
+ if (ndev == NULL)
+ return NULL;

- rwlock_init(&ndev->lock);
- ndev->dev = dev;
- memcpy(&ndev->cnf, &ipv6_devconf_dflt, sizeof(ndev->cnf));
- ndev->cnf.mtu6 = dev->mtu;
- ndev->cnf.sysctl = NULL;
- ndev->nd_parms = neigh_parms_alloc(dev, &nd_tbl);
- if (ndev->nd_parms == NULL) {
- kfree(ndev);
- return NULL;
- }
- /* We refer to the device */
- dev_hold(dev);
+ rwlock_init(&ndev->lock);
+ ndev->dev = dev;
+ memcpy(&ndev->cnf, &ipv6_devconf_dflt, sizeof(ndev->cnf));
+ ndev->cnf.mtu6 = dev->mtu;
+ ndev->cnf.sysctl = NULL;
+ ndev->nd_parms = neigh_parms_alloc(dev, &nd_tbl);
+ if (ndev->nd_parms == NULL) {
+ kfree(ndev);
+ return NULL;
+ }
+ /* We refer to the device */
+ dev_hold(dev);

- if (snmp6_alloc_dev(ndev) < 0) {
- ADBG((KERN_WARNING
- "%s(): cannot allocate memory for statistics; dev=%s.\n",
- __FUNCTION__, dev->name));
- neigh_parms_release(&nd_tbl, ndev->nd_parms);
- ndev->dead = 1;
- in6_dev_finish_destroy(ndev);
- return NULL;
- }
+ if (snmp6_alloc_dev(ndev) < 0) {
+ ADBG((KERN_WARNING
+ "%s(): cannot allocate memory for statistics; dev=%s.\n",
+ __FUNCTION__, dev->name));
+ neigh_parms_release(&nd_tbl, ndev->nd_parms);
+ ndev->dead = 1;
+ in6_dev_finish_destroy(ndev);
+ return NULL;
+ }

- if (snmp6_register_dev(ndev) < 0) {
- ADBG((KERN_WARNING
- "%s(): cannot create /proc/net/dev_snmp6/%s\n",
- __FUNCTION__, dev->name));
- neigh_parms_release(&nd_tbl, ndev->nd_parms);
- ndev->dead = 1;
- in6_dev_finish_destroy(ndev);
- return NULL;
- }
+ if (snmp6_register_dev(ndev) < 0) {
+ ADBG((KERN_WARNING
+ "%s(): cannot create /proc/net/dev_snmp6/%s\n",
+ __FUNCTION__, dev->name));
+ neigh_parms_release(&nd_tbl, ndev->nd_parms);
+ ndev->dead = 1;
+ in6_dev_finish_destroy(ndev);
+ return NULL;
+ }

- /* One reference from device. We must do this before
- * we invoke __ipv6_regen_rndid().
- */
- in6_dev_hold(ndev);
+ /* One reference from device. We must do this before
+ * we invoke __ipv6_regen_rndid().
+ */
+ in6_dev_hold(ndev);

#ifdef CONFIG_IPV6_PRIVACY
- get_random_bytes(ndev->rndid, sizeof(ndev->rndid));
- get_random_bytes(ndev->entropy, sizeof(ndev->entropy));
- init_timer(&ndev->regen_timer);
- ndev->regen_timer.function = ipv6_regen_rndid;
- ndev->regen_timer.data = (unsigned long) ndev;
- if ((dev->flags&IFF_LOOPBACK) ||
- dev->type == ARPHRD_TUNNEL ||
- dev->type == ARPHRD_NONE ||
- dev->type == ARPHRD_SIT) {
- printk(KERN_INFO
- "%s: Disabled Privacy Extensions\n",
- dev->name);
- ndev->cnf.use_tempaddr = -1;
- } else {
- in6_dev_hold(ndev);
- ipv6_regen_rndid((unsigned long) ndev);
- }
+ get_random_bytes(ndev->rndid, sizeof(ndev->rndid));
+ get_random_bytes(ndev->entropy, sizeof(ndev->entropy));
+ init_timer(&ndev->regen_timer);
+ ndev->regen_timer.function = ipv6_regen_rndid;
+ ndev->regen_timer.data = (unsigned long) ndev;
+ if ((dev->flags&IFF_LOOPBACK) ||
+ dev->type == ARPHRD_TUNNEL ||
+ dev->type == ARPHRD_NONE ||
+ dev->type == ARPHRD_SIT) {
+ printk(KERN_INFO
+ "%s: Disabled Privacy Extensions\n",
+ dev->name);
+ ndev->cnf.use_tempaddr = -1;
+ } else {
+ in6_dev_hold(ndev);
+ ipv6_regen_rndid((unsigned long) ndev);
+ }
#endif

- if (netif_carrier_ok(dev))
- ndev->if_flags |= IF_READY;
+ if (netif_carrier_ok(dev))
+ ndev->if_flags |= IF_READY;

- write_lock_bh(&addrconf_lock);
- dev->ip6_ptr = ndev;
- write_unlock_bh(&addrconf_lock);
+ write_lock_bh(&addrconf_lock);
+ dev->ip6_ptr = ndev;
+ write_unlock_bh(&addrconf_lock);

- ipv6_mc_init_dev(ndev);
- ndev->tstamp = jiffies;
+ ipv6_mc_init_dev(ndev);
+ ndev->tstamp = jiffies;
#ifdef CONFIG_SYSCTL
- neigh_sysctl_register(dev, ndev->nd_parms, NET_IPV6,
- NET_IPV6_NEIGH, "ipv6",
- &ndisc_ifinfo_sysctl_change,
- NULL);
- addrconf_sysctl_register(ndev, &ndev->cnf);
+ neigh_sysctl_register(dev, ndev->nd_parms, NET_IPV6,
+ NET_IPV6_NEIGH, "ipv6",
+ &ndisc_ifinfo_sysctl_change,
+ NULL);
+ addrconf_sysctl_register(ndev, &ndev->cnf);
#endif
- }
return ndev;
}

@@ -524,7 +523,7 @@ ipv6_add_addr(struct inet6_dev *idev, co
goto out;
}

- ifa = kmalloc(sizeof(struct inet6_ifaddr), GFP_ATOMIC);
+ ifa = kzalloc(sizeof(struct inet6_ifaddr), GFP_ATOMIC);

if (ifa == NULL) {
ADBG(("ipv6_add_addr: malloc failed\n"));
@@ -538,7 +537,6 @@ ipv6_add_addr(struct inet6_dev *idev, co
goto out;
}

- memset(ifa, 0, sizeof(struct inet6_ifaddr));
ipv6_addr_copy(&ifa->addr, addr);

spin_lock_init(&ifa->lock);
@@ -2668,11 +2666,10 @@ static int if6_seq_open(struct inode *in
{
struct seq_file *seq;
int rc = -ENOMEM;
- struct if6_iter_state *s = kmalloc(sizeof(*s), GFP_KERNEL);
+ struct if6_iter_state *s = kzalloc(sizeof(*s), GFP_KERNEL);

if (!s)
goto out;
- memset(s, 0, sizeof(*s));

rc = seq_open(file, &if6_seq_ops);
if (rc)


2006-02-11 17:09:42

by YOSHIFUJI Hideaki

[permalink] [raw]
Subject: Re: [PATCH] IPv6: Cleanups for net/ipv6/addrconf.c (kzalloc, early exit) v2

Hello.

In article <[email protected]> (at Sat, 11 Feb 2006 17:37:18 +0100), Ingo Oeser <[email protected]> says:

> From: Ingo Oeser <[email protected]>
>
> Here are some possible (and trivial) cleanups.
> - use kzalloc() where possible
> - remove unused label
> - invert allocation failure test like
:
> Signed-off-by: Ingo Oeser <[email protected]>
Acked-by: YOSHIFUJI Hideaki <[email protected]>

--yoshfuji

2006-03-10 11:02:57

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] IPv6: Cleanups for net/ipv6/addrconf.c (kzalloc, early exit) v2

From: YOSHIFUJI Hideaki <[email protected]>
Date: Sun, 12 Feb 2006 02:11:03 +0900 (JST)

> In article <[email protected]> (at Sat, 11 Feb 2006 17:37:18 +0100), Ingo Oeser <[email protected]> says:
>
> > From: Ingo Oeser <[email protected]>
> >
> > Here are some possible (and trivial) cleanups.
> > - use kzalloc() where possible
> > - remove unused label
> > - invert allocation failure test like
> :
> > Signed-off-by: Ingo Oeser <[email protected]>
> Acked-by: YOSHIFUJI Hideaki <[email protected]>

This patch no longer applied cleanly, Ingo can you generate
a fresh version of your patch against my net-2.6.16 tree?

Thanks a lot!

2006-03-10 22:35:46

by Ingo Oeser

[permalink] [raw]
Subject: Re: [PATCH] IPv6: Cleanups for net/ipv6/addrconf.c (kzalloc, early exit) v2

From: Ingo Oeser <[email protected]>

Here are some possible (and trivial) cleanups.
- use kzalloc() where possible
- invert allocation failure test like
if (object) {
/* Rest of function here */
}
to

if (object == NULL)
return NULL;

/* Rest of function here */

Signed-off-by: Ingo Oeser <[email protected]>
Acked-by: YOSHIFUJI Hideaki <[email protected]>

---
Hello Dave,

On Friday, 10. March 2006 12:02, David S. Miller wrote:
> This patch no longer applied cleanly, Ingo can you generate
> a fresh version of your patch against my net-2.6.16 tree?

Done!

I rebased it, but against net-2.6.17, since it did apply cleanly to
net-2.6 and I didn't find the tree for net-2.6.16 as requested.

Hope I got it right :-)

Regards

Ingo Oeser

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8df9eb9..395b3f8 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -341,84 +341,83 @@ static struct inet6_dev * ipv6_add_dev(s
if (dev->mtu < IPV6_MIN_MTU)
return NULL;

- ndev = kmalloc(sizeof(struct inet6_dev), GFP_KERNEL);
-
- if (ndev) {
- memset(ndev, 0, sizeof(struct inet6_dev));
-
- rwlock_init(&ndev->lock);
- ndev->dev = dev;
- memcpy(&ndev->cnf, &ipv6_devconf_dflt, sizeof(ndev->cnf));
- ndev->cnf.mtu6 = dev->mtu;
- ndev->cnf.sysctl = NULL;
- ndev->nd_parms = neigh_parms_alloc(dev, &nd_tbl);
- if (ndev->nd_parms == NULL) {
- kfree(ndev);
- return NULL;
- }
- /* We refer to the device */
- dev_hold(dev);
+ ndev = kzalloc(sizeof(struct inet6_dev), GFP_KERNEL);
+
+ if (ndev == NULL)
+ return NULL;
+
+ rwlock_init(&ndev->lock);
+ ndev->dev = dev;
+ memcpy(&ndev->cnf, &ipv6_devconf_dflt, sizeof(ndev->cnf));
+ ndev->cnf.mtu6 = dev->mtu;
+ ndev->cnf.sysctl = NULL;
+ ndev->nd_parms = neigh_parms_alloc(dev, &nd_tbl);
+ if (ndev->nd_parms == NULL) {
+ kfree(ndev);
+ return NULL;
+ }
+ /* We refer to the device */
+ dev_hold(dev);

- if (snmp6_alloc_dev(ndev) < 0) {
- ADBG((KERN_WARNING
- "%s(): cannot allocate memory for statistics; dev=%s.\n",
- __FUNCTION__, dev->name));
- neigh_parms_release(&nd_tbl, ndev->nd_parms);
- ndev->dead = 1;
- in6_dev_finish_destroy(ndev);
- return NULL;
- }
+ if (snmp6_alloc_dev(ndev) < 0) {
+ ADBG((KERN_WARNING
+ "%s(): cannot allocate memory for statistics; dev=%s.\n",
+ __FUNCTION__, dev->name));
+ neigh_parms_release(&nd_tbl, ndev->nd_parms);
+ ndev->dead = 1;
+ in6_dev_finish_destroy(ndev);
+ return NULL;
+ }

- if (snmp6_register_dev(ndev) < 0) {
- ADBG((KERN_WARNING
- "%s(): cannot create /proc/net/dev_snmp6/%s\n",
- __FUNCTION__, dev->name));
- neigh_parms_release(&nd_tbl, ndev->nd_parms);
- ndev->dead = 1;
- in6_dev_finish_destroy(ndev);
- return NULL;
- }
+ if (snmp6_register_dev(ndev) < 0) {
+ ADBG((KERN_WARNING
+ "%s(): cannot create /proc/net/dev_snmp6/%s\n",
+ __FUNCTION__, dev->name));
+ neigh_parms_release(&nd_tbl, ndev->nd_parms);
+ ndev->dead = 1;
+ in6_dev_finish_destroy(ndev);
+ return NULL;
+ }

- /* One reference from device. We must do this before
- * we invoke __ipv6_regen_rndid().
- */
- in6_dev_hold(ndev);
+ /* One reference from device. We must do this before
+ * we invoke __ipv6_regen_rndid().
+ */
+ in6_dev_hold(ndev);

#ifdef CONFIG_IPV6_PRIVACY
- init_timer(&ndev->regen_timer);
- ndev->regen_timer.function = ipv6_regen_rndid;
- ndev->regen_timer.data = (unsigned long) ndev;
- if ((dev->flags&IFF_LOOPBACK) ||
- dev->type == ARPHRD_TUNNEL ||
- dev->type == ARPHRD_NONE ||
- dev->type == ARPHRD_SIT) {
- printk(KERN_INFO
- "%s: Disabled Privacy Extensions\n",
- dev->name);
- ndev->cnf.use_tempaddr = -1;
- } else {
- in6_dev_hold(ndev);
- ipv6_regen_rndid((unsigned long) ndev);
- }
+ init_timer(&ndev->regen_timer);
+ ndev->regen_timer.function = ipv6_regen_rndid;
+ ndev->regen_timer.data = (unsigned long) ndev;
+ if ((dev->flags&IFF_LOOPBACK) ||
+ dev->type == ARPHRD_TUNNEL ||
+ dev->type == ARPHRD_NONE ||
+ dev->type == ARPHRD_SIT) {
+ printk(KERN_INFO
+ "%s: Disabled Privacy Extensions\n",
+ dev->name);
+ ndev->cnf.use_tempaddr = -1;
+ } else {
+ in6_dev_hold(ndev);
+ ipv6_regen_rndid((unsigned long) ndev);
+ }
#endif

- if (netif_carrier_ok(dev))
- ndev->if_flags |= IF_READY;
+ if (netif_carrier_ok(dev))
+ ndev->if_flags |= IF_READY;

- write_lock_bh(&addrconf_lock);
- dev->ip6_ptr = ndev;
- write_unlock_bh(&addrconf_lock);
+ write_lock_bh(&addrconf_lock);
+ dev->ip6_ptr = ndev;
+ write_unlock_bh(&addrconf_lock);

- ipv6_mc_init_dev(ndev);
- ndev->tstamp = jiffies;
+ ipv6_mc_init_dev(ndev);
+ ndev->tstamp = jiffies;
#ifdef CONFIG_SYSCTL
- neigh_sysctl_register(dev, ndev->nd_parms, NET_IPV6,
- NET_IPV6_NEIGH, "ipv6",
- &ndisc_ifinfo_sysctl_change,
- NULL);
- addrconf_sysctl_register(ndev, &ndev->cnf);
+ neigh_sysctl_register(dev, ndev->nd_parms, NET_IPV6,
+ NET_IPV6_NEIGH, "ipv6",
+ &ndisc_ifinfo_sysctl_change,
+ NULL);
+ addrconf_sysctl_register(ndev, &ndev->cnf);
#endif
- }
return ndev;
}

@@ -536,7 +535,7 @@ ipv6_add_addr(struct inet6_dev *idev, co
goto out;
}

- ifa = kmalloc(sizeof(struct inet6_ifaddr), GFP_ATOMIC);
+ ifa = kzalloc(sizeof(struct inet6_ifaddr), GFP_ATOMIC);

if (ifa == NULL) {
ADBG(("ipv6_add_addr: malloc failed\n"));
@@ -550,7 +549,6 @@ ipv6_add_addr(struct inet6_dev *idev, co
goto out;
}

- memset(ifa, 0, sizeof(struct inet6_ifaddr));
ipv6_addr_copy(&ifa->addr, addr);

spin_lock_init(&ifa->lock);
@@ -2669,11 +2667,10 @@ static int if6_seq_open(struct inode *in
{
struct seq_file *seq;
int rc = -ENOMEM;
- struct if6_iter_state *s = kmalloc(sizeof(*s), GFP_KERNEL);
+ struct if6_iter_state *s = kzalloc(sizeof(*s), GFP_KERNEL);

if (!s)
goto out;
- memset(s, 0, sizeof(*s));

rc = seq_open(file, &if6_seq_ops);
if (rc)

2006-03-16 08:20:32

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] IPv6: Cleanups for net/ipv6/addrconf.c (kzalloc, early exit) v2

From: Ingo Oeser <[email protected]>
Date: Fri, 10 Mar 2006 23:34:26 +0100

> Here are some possible (and trivial) cleanups.
> - use kzalloc() where possible
> - invert allocation failure test like
> if (object) {
> /* Rest of function here */
> }
> to
>
> if (object == NULL)
> return NULL;
>
> /* Rest of function here */
>
> Signed-off-by: Ingo Oeser <[email protected]>
> Acked-by: YOSHIFUJI Hideaki <[email protected]>

Applied, thanks a lot Ingo.