| Message ID | 20250214-old_flags-v1-1-29096b9399a9@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: Remove redundant variable declaration in __dev_change_flags() | expand |
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>
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
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
> 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
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
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;
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(-) --- base-commit: 7a7e0197133d18cfd9931e7d3a842d0f5730223f change-id: 20250214-old_flags-528fe052471c Best regards,