[PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

[PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

[Breno Leitao]

The old_flags variable is declared twice in __dev_change_flags(),
causing a shadow variable warning. This patch fixes the issue by
removing the redundant declaration, reusing the existing old_flags
variable instead.

	net/core/dev.c:9225:16: warning: declaration shadows a local variable [-Wshadow]
	9225 |                 unsigned int old_flags = dev->flags;
	|                              ^
	net/core/dev.c:9185:15: note: previous declaration is here
	9185 |         unsigned int old_flags = dev->flags;
	|                      ^
	1 warning generated.

This change has no functional impact on the code, as the inner variable
does not affect the outer one. The fix simply eliminates the unnecessary
declaration and resolves the warning.

Fixes: 991fb3f74c142e ("dev: always advertise rx_flags changes via netlink")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d5ab9a4b318ea4926c200ef20dae01eaafa18c6b..cd2474a138201e6ee86acf39ca425d57d8d2e9b4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9182,7 +9182,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
 
 	if ((flags ^ dev->gflags) & IFF_PROMISC) {
 		int inc = (flags & IFF_PROMISC) ? 1 : -1;
-		unsigned int old_flags = dev->flags;
+		old_flags = dev->flags;
 
 		dev->gflags ^= IFF_PROMISC;
 

---
base-commit: 7a7e0197133d18cfd9931e7d3a842d0f5730223f
change-id: 20250214-old_flags-528fe052471c

Best regards,
-- 
Breno Leitao <leitao@debian.org>

Re: [PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

[Mateusz Polchlopek]

On 2/14/2025 1:47 PM, Breno Leitao wrote:
> The old_flags variable is declared twice in __dev_change_flags(),
> causing a shadow variable warning. This patch fixes the issue by
> removing the redundant declaration, reusing the existing old_flags
> variable instead.
> 
> 	net/core/dev.c:9225:16: warning: declaration shadows a local variable [-Wshadow]
> 	9225 |                 unsigned int old_flags = dev->flags;
> 	|                              ^
> 	net/core/dev.c:9185:15: note: previous declaration is here
> 	9185 |         unsigned int old_flags = dev->flags;
> 	|                      ^
> 	1 warning generated.
> 
> This change has no functional impact on the code, as the inner variable
> does not affect the outer one. The fix simply eliminates the unnecessary
> declaration and resolves the warning.
> 
> Fixes: 991fb3f74c142e ("dev: always advertise rx_flags changes via netlink")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>   net/core/dev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d5ab9a4b318ea4926c200ef20dae01eaafa18c6b..cd2474a138201e6ee86acf39ca425d57d8d2e9b4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9182,7 +9182,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
>   
>   	if ((flags ^ dev->gflags) & IFF_PROMISC) {
>   		int inc = (flags & IFF_PROMISC) ? 1 : -1;
> -		unsigned int old_flags = dev->flags;
> +		old_flags = dev->flags;
>   
>   		dev->gflags ^= IFF_PROMISC;
>   
> 
> ---
> base-commit: 7a7e0197133d18cfd9931e7d3a842d0f5730223f
> change-id: 20250214-old_flags-528fe052471c
> 
> Best regards,

Good change but it has to be tagged to net and not net-next. Please
resend but add also my RB tag, thanks.

Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>

Re: [PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

[Andrew Lunn]

On Fri, Feb 14, 2025 at 04:47:49AM -0800, Breno Leitao wrote:
> The old_flags variable is declared twice in __dev_change_flags(),
> causing a shadow variable warning. This patch fixes the issue by
> removing the redundant declaration, reusing the existing old_flags
> variable instead.
> 
> 	net/core/dev.c:9225:16: warning: declaration shadows a local variable [-Wshadow]
> 	9225 |                 unsigned int old_flags = dev->flags;
> 	|                              ^
> 	net/core/dev.c:9185:15: note: previous declaration is here
> 	9185 |         unsigned int old_flags = dev->flags;
> 	|                      ^
> 	1 warning generated.
> 
> This change has no functional impact on the code, as the inner variable
> does not affect the outer one. The fix simply eliminates the unnecessary
> declaration and resolves the warning.

I'm not a compiler person... but there might be some subtlety here:


int __dev_change_flags(struct net_device *dev, unsigned int flags,
		       struct netlink_ext_ack *extack)
{
	unsigned int old_flags = dev->flags;
	int ret;

This old_flags gets the value of flags at the time of entry into the
function.

...

	if ((old_flags ^ flags) & IFF_UP) {
		if (old_flags & IFF_UP)
			__dev_close(dev);
		else
			ret = __dev_open(dev, extack);
	}

If you dig down into __dev_close(dev) you find

		dev->flags &= ~IFF_UP;

then

...

	if ((flags ^ dev->gflags) & IFF_PROMISC) {
		int inc = (flags & IFF_PROMISC) ? 1 : -1;
		unsigned int old_flags = dev->flags;

This inner old_flags now has the IFF_UP removed, and so is different
to the outer old_flags.

The outer old_flags is not used after this point, so in the end it
might not matter, but that fact i felt i needed to look deeper at the
code suggests the commit message needs expanding to include more
analyses.

> Fixes: 991fb3f74c142e ("dev: always advertise rx_flags changes via netlink")

I suppose there is also a danger here this code has at some point in
the past has been refactored, such that the outer old_flags was used
at some point? Backporting this patch could then break something?  Did
you check for this? Again, a comment in the commit message that you
have checked this is safe to backport would be nice.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

[Breno Leitao]

hello Andew,

On Fri, Feb 14, 2025 at 02:33:45PM +0100, Andrew Lunn wrote:
> On Fri, Feb 14, 2025 at 04:47:49AM -0800, Breno Leitao wrote:
> > The old_flags variable is declared twice in __dev_change_flags(),
> > causing a shadow variable warning. This patch fixes the issue by
> > removing the redundant declaration, reusing the existing old_flags
> > variable instead.
> > 
> > 	net/core/dev.c:9225:16: warning: declaration shadows a local variable [-Wshadow]
> > 	9225 |                 unsigned int old_flags = dev->flags;
> > 	|                              ^
> > 	net/core/dev.c:9185:15: note: previous declaration is here
> > 	9185 |         unsigned int old_flags = dev->flags;
> > 	|                      ^
> > 	1 warning generated.
> > 
> > This change has no functional impact on the code, as the inner variable
> > does not affect the outer one. The fix simply eliminates the unnecessary
> > declaration and resolves the warning.
> 
> I'm not a compiler person... but there might be some subtlety here:
> 
> 
> int __dev_change_flags(struct net_device *dev, unsigned int flags,
> 		       struct netlink_ext_ack *extack)
> {
> 	unsigned int old_flags = dev->flags;
> 	int ret;
> 
> This old_flags gets the value of flags at the time of entry into the
> function.
> 
> ...
> 
> 	if ((old_flags ^ flags) & IFF_UP) {
> 		if (old_flags & IFF_UP)
> 			__dev_close(dev);
> 		else
> 			ret = __dev_open(dev, extack);
> 	}
> 
> If you dig down into __dev_close(dev) you find
> 
> 		dev->flags &= ~IFF_UP;
> 
> then
> 
> ...
> 
> 	if ((flags ^ dev->gflags) & IFF_PROMISC) {
> 		int inc = (flags & IFF_PROMISC) ? 1 : -1;
> 		unsigned int old_flags = dev->flags;
> 
> This inner old_flags now has the IFF_UP removed, and so is different
> to the outer old_flags.
> 
> The outer old_flags is not used after this point, so in the end it
> might not matter, but that fact i felt i needed to look deeper at the
> code suggests the commit message needs expanding to include more
> analyses.

Right, I have analyzed this when creating this fix. I was wondering if
I need to rename the inner old_flags or just reuse it, and I added it in
the commit message:

 > This change has no functional impact on the code, as the inner variable
 > does not affect the outer one.

But I agree with you, if you needed to look at it, it means the message
is NOT good enough. I will update it.

> > Fixes: 991fb3f74c142e ("dev: always advertise rx_flags changes via netlink")
> 
> I suppose there is also a danger here this code has at some point in
> the past has been refactored, such that the outer old_flags was used
> at some point? Backporting this patch could then break something?  Did
> you check for this? Again, a comment in the commit message that you
> have checked this is safe to backport would be nice.

I haven't look at this, and I don't think this should be backported,
thus, that is why I sent to net-next and didn't cc: stable.

That said, I don't think this should be backported, since it is not
a big deal. Shouldn't I add the Fixes: in such case?

Thanks for the review,
--breno

Re: [PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

[Andrew Lunn]

> But I agree with you, if you needed to look at it, it means the message
> is NOT good enough. I will update it.

Thanks.

> 
> > > Fixes: 991fb3f74c142e ("dev: always advertise rx_flags changes via netlink")
> > 
> > I suppose there is also a danger here this code has at some point in
> > the past has been refactored, such that the outer old_flags was used
> > at some point? Backporting this patch could then break something?  Did
> > you check for this? Again, a comment in the commit message that you
> > have checked this is safe to backport would be nice.
> 
> I haven't look at this, and I don't think this should be backported,
> thus, that is why I sent to net-next and didn't cc: stable.
> 
> That said, I don't think this should be backported, since it is not
> a big deal. Shouldn't I add the Fixes: in such case?

The danger of adding a Fixes: is that the ML bot will see the Fixes:
tag and might select it for backporting, even if we did not explicitly
queue it up for back porting. So i suggest dropping the tag.

    Andrew

Re: [PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

[Breno Leitao]

On Fri, Feb 14, 2025 at 04:02:10PM +0100, Andrew Lunn wrote:
> > But I agree with you, if you needed to look at it, it means the message
> > is NOT good enough. I will update it.
> 
> Thanks.
> 
> > 
> > > > Fixes: 991fb3f74c142e ("dev: always advertise rx_flags changes via netlink")
> > > 
> > > I suppose there is also a danger here this code has at some point in
> > > the past has been refactored, such that the outer old_flags was used
> > > at some point? Backporting this patch could then break something?  Did
> > > you check for this? Again, a comment in the commit message that you
> > > have checked this is safe to backport would be nice.
> > 
> > I haven't look at this, and I don't think this should be backported,
> > thus, that is why I sent to net-next and didn't cc: stable.
> > 
> > That said, I don't think this should be backported, since it is not
> > a big deal. Shouldn't I add the Fixes: in such case?
> 
> The danger of adding a Fixes: is that the ML bot will see the Fixes:
> tag and might select it for backporting, even if we did not explicitly
> queue it up for back porting. So i suggest dropping the tag.

Makes sense. I will drop the Fixes: tag and send a v2 to net-next.

Thanks Andrew,
--breno