[PATCH] devcoredump: avoid -Wempty-body warnings

[PATCH] devcoredump: avoid -Wempty-body warnings

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Cleaning out the last -Wempty-body warnings found some interesting
cases with empty macros, along with harmless warnings like this one:

drivers/base/devcoredump.c: In function 'dev_coredumpm':
drivers/base/devcoredump.c:297:56: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
  297 |                 /* nothing - symlink will be missing */;
      |                                                        ^
drivers/base/devcoredump.c:301:56: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
  301 |                 /* nothing - symlink will be missing */;
      |                                                        ^

Randy tried addressing this one before, and there were multiple
other ideas in that thread.

Pick up the one that Matthew Wilcox suggested by adding a
WARN_ON_ONCE() and a comment.

Link: https://lore.kernel.org/lkml/20200418184111.13401-8-rdunlap@infradead.org/
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/base/devcoredump.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index 9243468e2c99..88e0d931439e 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -292,13 +292,16 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 	if (device_add(&devcd->devcd_dev))
 		goto put_device;
 
-	if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj,
-			      "failing_device"))
-		/* nothing - symlink will be missing */;
+	/*
+	 * These should normally not fail, but there is no problem
+	 * continuing without the links, so just warn instead of
+	 * failing.
+	 */
+	WARN_ON_ONCE(sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj,
+				       "failing_device"));
 
-	if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
-			      "devcoredump"))
-		/* nothing - symlink will be missing */;
+	WARN_ON_ONCE(sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
+				       "devcoredump"));
 
 	INIT_DELAYED_WORK(&devcd->del_wk, devcd_del);
 	schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT);
-- 
2.29.2

Re: [PATCH] devcoredump: avoid -Wempty-body warnings

[Greg Kroah-Hartman]

On Mon, Mar 22, 2021 at 11:19:53AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Cleaning out the last -Wempty-body warnings found some interesting
> cases with empty macros, along with harmless warnings like this one:
> 
> drivers/base/devcoredump.c: In function 'dev_coredumpm':
> drivers/base/devcoredump.c:297:56: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
>   297 |                 /* nothing - symlink will be missing */;
>       |                                                        ^
> drivers/base/devcoredump.c:301:56: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
>   301 |                 /* nothing - symlink will be missing */;
>       |                                                        ^
> 
> Randy tried addressing this one before, and there were multiple
> other ideas in that thread.
> 
> Pick up the one that Matthew Wilcox suggested by adding a
> WARN_ON_ONCE() and a comment.
> 
> Link: https://lore.kernel.org/lkml/20200418184111.13401-8-rdunlap@infradead.org/
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/base/devcoredump.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> index 9243468e2c99..88e0d931439e 100644
> --- a/drivers/base/devcoredump.c
> +++ b/drivers/base/devcoredump.c
> @@ -292,13 +292,16 @@ void dev_coredumpm(struct device *dev, struct module *owner,
>  	if (device_add(&devcd->devcd_dev))
>  		goto put_device;
>  
> -	if (sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj,
> -			      "failing_device"))
> -		/* nothing - symlink will be missing */;
> +	/*
> +	 * These should normally not fail, but there is no problem
> +	 * continuing without the links, so just warn instead of
> +	 * failing.
> +	 */
> +	WARN_ON_ONCE(sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj,
> +				       "failing_device"));
>  
> -	if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
> -			      "devcoredump"))
> -		/* nothing - symlink will be missing */;
> +	WARN_ON_ONCE(sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
> +				       "devcoredump"));

We do not want to reboot machines that have panic-on-warn set, so if
this really needs a trace dump, please do that instead...

thanks,

greg k-h

Re: [PATCH] devcoredump: avoid -Wempty-body warnings

[Arnd Bergmann]

On Mon, Mar 22, 2021 at 11:29 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 22, 2021 at 11:19:53AM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > +      * These should normally not fail, but there is no problem
> > +      * continuing without the links, so just warn instead of
> > +      * failing.
> > +      */
> > +     WARN_ON_ONCE(sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj,
> > +                                    "failing_device"));
> >
> > -     if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
> > -                           "devcoredump"))
> > -             /* nothing - symlink will be missing */;
> > +     WARN_ON_ONCE(sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
> > +                                    "devcoredump"));
>
> We do not want to reboot machines that have panic-on-warn set,

Fair enough.

> so if this really needs a trace dump, please do that instead...

I don't think the backtrace is needed here, if it ever happens it's either
going to be -ENOMEM or completely reproducible. I can instead
do the cast to (void) that Linus suggested, or a simple if(...) dev_warn()
or dev_warn_once() to have some indication of the failure.

          Arnd

Re: [PATCH] devcoredump: avoid -Wempty-body warnings

[Greg Kroah-Hartman]

On Mon, Mar 22, 2021 at 12:22:11PM +0100, Arnd Bergmann wrote:
> On Mon, Mar 22, 2021 at 11:29 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Mar 22, 2021 at 11:19:53AM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > > +      * These should normally not fail, but there is no problem
> > > +      * continuing without the links, so just warn instead of
> > > +      * failing.
> > > +      */
> > > +     WARN_ON_ONCE(sysfs_create_link(&devcd->devcd_dev.kobj, &dev->kobj,
> > > +                                    "failing_device"));
> > >
> > > -     if (sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
> > > -                           "devcoredump"))
> > > -             /* nothing - symlink will be missing */;
> > > +     WARN_ON_ONCE(sysfs_create_link(&dev->kobj, &devcd->devcd_dev.kobj,
> > > +                                    "devcoredump"));
> >
> > We do not want to reboot machines that have panic-on-warn set,
> 
> Fair enough.
> 
> > so if this really needs a trace dump, please do that instead...
> 
> I don't think the backtrace is needed here, if it ever happens it's either
> going to be -ENOMEM or completely reproducible. I can instead
> do the cast to (void) that Linus suggested, or a simple if(...) dev_warn()
> or dev_warn_once() to have some indication of the failure.

dev_warn() should be fine I think.

thanks,

greg k-h