[RFC][PATCH 0/2] inet6: two cases of shadowing other variables

[RFC][PATCH 0/2] inet6: two cases of shadowing other variables

[Gerrit Renker]

Two cases of shadowing variables. 

Patches are not meant to be applied immediately, especially in the first
one I am not sure if the code is ok without adding a lock.

Can the IPv6 developers please have a look through:

Patch #1: Local variable shadows function argument. After renaming,
          the question arose whether the code is correct wrt locking.

          Please take a look at this code part, no lock is taken on
	  the pmc->idev. (There is similar code in ip6_mc_del_src()).

Patch #2: Several functions shadow a global variable.
	  Can be applied directly, but better to review wrt:
	  given that ipv6_sysctl_register() calls register_pernet_subsys(),
	  does it make sense to merge the one-row table with
	  ipv6_table_template[], or are there namespace issues?

[RFC][PATCH 1/2] mcastv6: Local variable shadows function argument

[Gerrit Renker]

The local variable 'idev' shadows the function argument 'idev' to
ip6_mc_add_src(). 

Reported-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/ipv6/mcast.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -2107,7 +2107,6 @@ static int ip6_mc_add_src(struct inet6_d
 		for (j=0; j<i; j++)
 			(void) ip6_mc_del1_src(pmc, sfmode, &psfsrc[i]);
 	} else if (isexclude != (pmc->mca_sfcount[MCAST_EXCLUDE] != 0)) {
-		struct inet6_dev *idev = pmc->idev;
 		struct ip6_sf_list *psf;
 
 		/* filter mode change */
@@ -2117,11 +2116,11 @@ static int ip6_mc_add_src(struct inet6_d
 			pmc->mca_sfmode = MCAST_INCLUDE;
 		/* else no filters; keep old mode for reports */
 
-		pmc->mca_crcount = idev->mc_qrv;
-		idev->mc_ifc_count = pmc->mca_crcount;
+		pmc->mca_crcount = pmc->idev->mc_qrv;
+		pmc->idev->mc_ifc_count = pmc->mca_crcount;
 		for (psf=pmc->mca_sources; psf; psf = psf->sf_next)
 			psf->sf_crcount = 0;
-		mld_ifc_event(idev);
+		mld_ifc_event(pmc->idev);
 	} else if (sf_setstate(pmc))
 		mld_ifc_event(idev);
 	spin_unlock_bh(&pmc->mca_lock);

[RFC][PATCH 2/2] inet6: sysctl pointers shadow global pointer

[Gerrit Renker]

net/ipv6/sysctl_net_ipv6.c:64:19:  warning: symbol 'ipv6_table' shadows an earlier one
net/ipv6/sysctl_net_ipv6.c:107:19: warning: symbol 'ipv6_table' shadows an earlier one
net/ipv6/sysctl_net_ipv6.c:43:18:  originally declared here

The only place where 'ipv6_table' is not shadowed is in ipv6_sysctl_register().

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/ipv6/sysctl_net_ipv6.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -40,7 +40,7 @@ static ctl_table ipv6_table_template[] =
 	{ .ctl_name = 0 }
 };
 
-static ctl_table ipv6_table[] = {
+static ctl_table ipv6_rotable[] = {
 	{
 		.ctl_name	= NET_IPV6_MLD_MAX_MSF,
 		.procname	= "mld_max_msf",
@@ -130,7 +130,7 @@ int ipv6_sysctl_register(void)
 {
 	int err = -ENOMEM;
 
-	ip6_header = register_net_sysctl_rotable(net_ipv6_ctl_path, ipv6_table);
+	ip6_header = register_net_sysctl_rotable(net_ipv6_ctl_path, ipv6_rotable);
 	if (ip6_header == NULL)
 		goto out;
 

Re: [RFC][PATCH 1/2] mcastv6: Local variable shadows function argument

[David Stevens]

pmc->idev should be equal to idev, so you should be able
to simply remove the extra declaration and leave the other
code as it is-- i.e., a 1-line change.

                                        +-DLS

Re: [RFC][PATCH 1/2] mcastv6: Local variable shadows function argument

[Gerrit Renker]

| pmc->idev should be equal to idev, so you should be able
| to simply remove the extra declaration and leave the other
| code as it is-- i.e., a 1-line change.
| 
Thank you for checking - so it would look like this?
If this is correct, it would resolve shadowing the passed idev:

--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -2107,7 +2107,6 @@ static int ip6_mc_add_src(struct inet6_d
 		for (j=0; j<i; j++)
 			(void) ip6_mc_del1_src(pmc, sfmode, &psfsrc[i]);
 	} else if (isexclude != (pmc->mca_sfcount[MCAST_EXCLUDE] != 0)) {
-		struct inet6_dev *idev = pmc->idev;
 		struct ip6_sf_list *psf;
 
 		/* filter mode change */

Re: [RFC][PATCH 1/2] mcastv6: Local variable shadows function argument

[David Stevens]

Yes, looks good.

                +-DLS


Acked-by: David L Stevens <dlstevens@us.ibm.com>

[Resent][PATCH 1/1] inet6: functions shadow global variable

[Gerrit Renker]

There was no further response on the RFC, I am resending it as a fix.

>>>>>>>>>>>>>>>>>>>>>>>>> Patch v1 (resent) <<<<<<<<<<<<<<<<<<<<<<<<
inet6: local functions shadow global pointer

This renames away a variable clash:
 * ipv6_table[] is declared as a static global table;
 * ipv6_sysctl_net_init() uses ipv6_table to refer/destroy dynamic memory;
 * ipv6_sysctl_net_exit() also uses ipv6_table for the same purpose;
 * both the two last functions call kfree() on ipv6_table.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/ipv6/sysctl_net_ipv6.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -40,7 +40,7 @@ static ctl_table ipv6_table_template[] =
 	{ .ctl_name = 0 }
 };
 
-static ctl_table ipv6_table[] = {
+static ctl_table ipv6_rotable[] = {
 	{
 		.ctl_name	= NET_IPV6_MLD_MAX_MSF,
 		.procname	= "mld_max_msf",
@@ -130,7 +130,7 @@ int ipv6_sysctl_register(void)
 {
 	int err = -ENOMEM;
 
-	ip6_header = register_net_sysctl_rotable(net_ipv6_ctl_path, ipv6_table);
+	ip6_header = register_net_sysctl_rotable(net_ipv6_ctl_path, ipv6_rotable);
 	if (ip6_header == NULL)
 		goto out;
 

Re: [Resent][PATCH 1/1] inet6: functions shadow global variable

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Tue, 28 Jul 2009 21:48:07 +0200

> inet6: local functions shadow global pointer
> 
> This renames away a variable clash:
>  * ipv6_table[] is declared as a static global table;
>  * ipv6_sysctl_net_init() uses ipv6_table to refer/destroy dynamic memory;
>  * ipv6_sysctl_net_exit() also uses ipv6_table for the same purpose;
>  * both the two last functions call kfree() on ipv6_table.
> 
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

Applied.