| Message ID | 20200418184111.13401-8-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fix -Wempty-body build warnings | expand |
On Sat, Apr 18, 2020 at 11:41:09AM -0700, Randy Dunlap wrote: > @@ -294,11 +295,11 @@ void dev_coredumpm(struct device *dev, s > > if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > "failing_device")) > - /* nothing - symlink will be missing */; > + do_empty(); /* nothing - symlink will be missing */ > > if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj, > "devcoredump")) > - /* nothing - symlink will be missing */; > + do_empty(); /* nothing - symlink will be missing */ > > INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); > schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT); Could just remove the 'if's? + sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, + "failing_device");
On 4/18/20 11:50 AM, Matthew Wilcox wrote: > On Sat, Apr 18, 2020 at 11:41:09AM -0700, Randy Dunlap wrote: >> @@ -294,11 +295,11 @@ void dev_coredumpm(struct device *dev, s >> >> if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, >> "failing_device")) >> - /* nothing - symlink will be missing */; >> + do_empty(); /* nothing - symlink will be missing */ >> >> if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj, >> "devcoredump")) >> - /* nothing - symlink will be missing */; >> + do_empty(); /* nothing - symlink will be missing */ >> >> INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); >> schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT); > > Could just remove the 'if's? > > + sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > + "failing_device"); > OK. thanks.
On Sat, 2020-04-18 at 11:53 -0700, Randy Dunlap wrote: > On 4/18/20 11:50 AM, Matthew Wilcox wrote: > > On Sat, Apr 18, 2020 at 11:41:09AM -0700, Randy Dunlap wrote: > > > @@ -294,11 +295,11 @@ void dev_coredumpm(struct device *dev, s > > > > > > if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > > > "failing_device")) > > > - /* nothing - symlink will be missing */; > > > + do_empty(); /* nothing - symlink will be missing */ > > > > > > if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj, > > > "devcoredump")) > > > - /* nothing - symlink will be missing */; > > > + do_empty(); /* nothing - symlink will be missing */ > > > > > > INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); > > > schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT); > > > > Could just remove the 'if's? > > > > + sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > > + "failing_device"); > > > > OK. sysfs_create_link is __must_check
On Sat, Apr 18, 2020 at 11:55:05AM -0700, Joe Perches wrote: > On Sat, 2020-04-18 at 11:53 -0700, Randy Dunlap wrote: > > On 4/18/20 11:50 AM, Matthew Wilcox wrote: > > > On Sat, Apr 18, 2020 at 11:41:09AM -0700, Randy Dunlap wrote: > > > > @@ -294,11 +295,11 @@ void dev_coredumpm(struct device *dev, s > > > > > > > > if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > > > > "failing_device")) > > > > - /* nothing - symlink will be missing */; > > > > + do_empty(); /* nothing - symlink will be missing */ > > > > > > > > if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj, > > > > "devcoredump")) > > > > - /* nothing - symlink will be missing */; > > > > + do_empty(); /* nothing - symlink will be missing */ > > > > > > > > INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); > > > > schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT); > > > > > > Could just remove the 'if's? > > > > > > + sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > > > + "failing_device"); > > > > > > > OK. > > sysfs_create_link is __must_check Oh, I missed the declaration -- I just saw the definition. This is a situation where __must_check hurts us and it should be removed. Or this code is wrong and it should be WARN(sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, "failing_device"); like drivers/pci/controller/vmd.c and drivers/i2c/i2c-mux.c Either way, the do_empty() construct feels like the wrong way of covering up the warning.
On Sat, Apr 18, 2020 at 11:57 AM Joe Perches <joe@perches.com> wrote: > > sysfs_create_link is __must_check The way to handle __must_check if you really really don't want to test and have good reasons is (a) add a big comment about why this case ostensibly doesn't need the check (b) cast a test of it to '(void)' or something (I guess we could add a helper for this). So something like /* We will always clean up, we don't care whether this fails or succeeds */ (void)!!sysfs_create_link(...) There are other alternatives (like using WARN_ON_ONCE() instead, for example). So it depends on the code. Which is why that comment is important to show why the code chose that option. However, I wonder if in this case we should just remove the __must_check. Greg? It goes back a long long time. Particularly for the "nowarn" version of that function. I'm not seeing why you'd have to care, particularly if you don't even care about the link already existing.. Linus
On Sat, 2020-04-18 at 12:13 -0700, Matthew Wilcox wrote: > > > > > > if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > > > > > "failing_device")) > > > > > - /* nothing - symlink will be missing */; > > > > > + do_empty(); /* nothing - symlink will be missing */ > > > > > > > > > > if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj, > > > > > "devcoredump")) > > > > > - /* nothing - symlink will be missing */; > > > > > + do_empty(); /* nothing - symlink will be missing */ > > > > > > > > > > INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); > > > > > schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT); > > > > > > > > Could just remove the 'if's? > > > > > > > > + sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > > > > + "failing_device"); > > > > > > > > > > OK. > > > > sysfs_create_link is __must_check > > Oh, I missed the declaration -- I just saw the definition. This is a > situation where __must_check hurts us and it should be removed. > > Or this code is wrong and it should be > > WARN(sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, > "failing_device"); Perhaps it should be. I didn't think it really mattered _that_ much if the symlink suddenly went missing, but OTOH I don't even know how it could possibly fail. johannes
On Sat, Apr 18, 2020 at 11:41:09AM -0700, Randy Dunlap wrote: > Fix gcc empty-body warning when -Wextra is used: > > ../drivers/base/devcoredump.c:297:42: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] > ../drivers/base/devcoredump.c:301:42: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Cc: Johannes Berg <johannes@sipsolutions.net> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > --- > drivers/base/devcoredump.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > --- linux-next-20200417.orig/drivers/base/devcoredump.c > +++ linux-next-20200417/drivers/base/devcoredump.c > @@ -9,6 +9,7 @@ > * > * Author: Johannes Berg <johannes@sipsolutions.net> > */ > +#include <linux/kernel.h> Why the need for this .h file being added for reformatting the code? thanks, greg k-h
On Sun, Apr 19, 2020 at 08:02:47AM +0200, Greg Kroah-Hartman wrote: > On Sat, Apr 18, 2020 at 11:41:09AM -0700, Randy Dunlap wrote: > > Fix gcc empty-body warning when -Wextra is used: > > > > ../drivers/base/devcoredump.c:297:42: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] > > ../drivers/base/devcoredump.c:301:42: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] > > > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > > Cc: Johannes Berg <johannes@sipsolutions.net> > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > --- > > drivers/base/devcoredump.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > --- linux-next-20200417.orig/drivers/base/devcoredump.c > > +++ linux-next-20200417/drivers/base/devcoredump.c > > @@ -9,6 +9,7 @@ > > * > > * Author: Johannes Berg <johannes@sipsolutions.net> > > */ > > +#include <linux/kernel.h> > > Why the need for this .h file being added for reformatting the code? Ah, the function you add, nevermind, need more coffee...
On Sat, Apr 18, 2020 at 12:15:57PM -0700, Linus Torvalds wrote: > On Sat, Apr 18, 2020 at 11:57 AM Joe Perches <joe@perches.com> wrote: > > > > sysfs_create_link is __must_check > > The way to handle __must_check if you really really don't want to test > and have good reasons is > > (a) add a big comment about why this case ostensibly doesn't need the check > > (b) cast a test of it to '(void)' or something (I guess we could add > a helper for this). So something like > > /* We will always clean up, we don't care whether this fails > or succeeds */ > (void)!!sysfs_create_link(...) > > There are other alternatives (like using WARN_ON_ONCE() instead, for > example). So it depends on the code. Which is why that comment is > important to show why the code chose that option. > > However, I wonder if in this case we should just remove the > __must_check. Greg? It goes back a long long time. Yeah, maybe it is time to remove it, the gyrations people go through to remove the warning when they "know" they are doing it right feels pretty bad compared to forcing people to check things for "normal" calls to the function. thanks, greg k-h
--- linux-next-20200417.orig/drivers/base/devcoredump.c +++ linux-next-20200417/drivers/base/devcoredump.c @@ -9,6 +9,7 @@ * * Author: Johannes Berg <johannes@sipsolutions.net> */ +#include <linux/kernel.h> #include <linux/module.h> #include <linux/device.h> #include <linux/devcoredump.h> @@ -294,11 +295,11 @@ void dev_coredumpm(struct device *dev, s if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj, "failing_device")) - /* nothing - symlink will be missing */; + do_empty(); /* nothing - symlink will be missing */ if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj, "devcoredump")) - /* nothing - symlink will be missing */; + do_empty(); /* nothing - symlink will be missing */ INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT);
Fix gcc empty-body warning when -Wextra is used: ../drivers/base/devcoredump.c:297:42: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] ../drivers/base/devcoredump.c:301:42: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Cc: Johannes Berg <johannes@sipsolutions.net> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> --- drivers/base/devcoredump.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)