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