4:41 a.m.
Hi,
When -Wextra is used, gcc emits many warnings about an empty 'if' or
'else' body, like this:
../fs/posix_acl.c: In function ‘get_acl’:
../fs/posix_acl.c:127:22: warning: suggest braces around empty body in an ‘if’ statement
[-Wempty-body]
/* fall through */ ;
^
To quieten these warnings, add a new macro "do_empty()".
I originally wanted to use do_nothing(), but that's already in use.
It would sorta be nice if "fallthrough" could be coerced for this
instead of using something like do_empty().
Or should we just use "{}" in place of ";"?
This causes some odd coding style issue IMO. E.g., see this change:
original:
if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED)
/* fall through */ ;
with new macro:
if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED)
do_empty(); /* fall through */
using {}:
if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED)
{} /* fall through */
or
{ /* fall through */ }
or even
if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) {
/* fall through */ }
or
if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) {
} /* fall through */
drivers/base/devcoredump.c | 5 +++--
drivers/dax/bus.c | 5 +++--
drivers/input/mouse/synaptics.c | 3 ++-
drivers/target/target_core_pscsi.c | 3 ++-
drivers/usb/core/sysfs.c | 2 +-
fs/nfsd/nfs4state.c | 3 ++-
fs/posix_acl.c | 2 +-
include/linux/kernel.h | 8 ++++++++
sound/drivers/vx/vx_core.c | 3 ++-
9 files changed, 24 insertions(+), 10 deletions(-)
4:41 a.m.
New subject: [RFC PATCH 1/9] kernel.h: add do_empty() macro
Add the do_empty() macro to silence gcc warnings about an empty body
following an "if" statement when -Wextra is used.
However, for debug printk calls that are being disabled, use either
no_printk() or pr_debug() [and optionally dynamic printk debugging]
instead.
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
Cc: Alexander Viro <viro(a)zeniv.linux.org.uk>
Cc: linux-fsdevel(a)vger.kernel.org
Cc: Dmitry Torokhov <dmitry.torokhov(a)gmail.com>
Cc: linux-input(a)vger.kernel.org
Cc: Jaroslav Kysela <perex(a)perex.cz>
Cc: Takashi Iwai <tiwai(a)suse.com>
Cc: alsa-devel(a)alsa-project.org
Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Cc: linux-usb(a)vger.kernel.org
Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
Cc: Chuck Lever <chuck.lever(a)oracle.com>
Cc: linux-nfs(a)vger.kernel.org
Cc: Johannes Berg <johannes(a)sipsolutions.net>
Cc: Dan Williams <dan.j.williams(a)intel.com>
Cc: Vishal Verma <vishal.l.verma(a)intel.com>
Cc: Dave Jiang <dave.jiang(a)intel.com>
Cc: linux-nvdimm(a)lists.01.org
Cc: "Martin K. Petersen" <martin.petersen(a)oracle.com>
Cc: linux-scsi(a)vger.kernel.org
Cc: target-devel(a)vger.kernel.org
Cc: Zzy Wysm <zzy(a)zzywysm.com>
---
include/linux/kernel.h | 8 ++++++++
1 file changed, 8 insertions(+)
--- linux-next-20200327.orig/include/linux/kernel.h
+++ linux-next-20200327/include/linux/kernel.h
@@ -40,6 +40,14 @@
#define READ 0
#define WRITE 1
+/*
+ * When using -Wextra, an "if" statement followed by an empty block
+ * (containing only a ';'), produces a warning from gcc:
+ * warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body]
+ * Replace the empty body with do_empty() to silence this warning.
+ */
+#define do_empty() do { } while (0)
+
/**
* ARRAY_SIZE - get the number of elements in array @arr
* @arr: array to be sized
4:41 a.m.
New subject: [PATCH 2/9] fs: fix empty-body warning in posix_acl.c
Fix gcc empty-body warning when -Wextra is used:
../fs/posix_acl.c: In function ‘get_acl’:
../fs/posix_acl.c:127:22: warning: suggest braces around empty body in an ‘if’ statement
[-Wempty-body]
/* fall through */ ;
^
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Alexander Viro <viro(a)zeniv.linux.org.uk>
Cc: linux-fsdevel(a)vger.kernel.org
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
---
fs/posix_acl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-next-20200327.orig/fs/posix_acl.c
+++ linux-next-20200327/fs/posix_acl.c
@@ -124,7 +124,7 @@ struct posix_acl *get_acl(struct inode *
* be an unlikely race.)
*/
if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED)
- /* fall through */ ;
+ do_empty(); /* fall through */
/*
* Normally, the ACL returned by ->get_acl will be cached.
4:41 a.m.
New subject: [PATCH 3/9] input: fix empty-body warning in synaptics.c
Fix gcc empty-body warning when -Wextra is used:
../drivers/input/mouse/synaptics.c: In function ‘synaptics_process_packet’:
../drivers/input/mouse/synaptics.c:1106:6: warning: suggest braces around empty body in an
‘if’ statement [-Wempty-body]
; /* Nothing, treat a pen as a single finger */
^
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Dmitry Torokhov <dmitry.torokhov(a)gmail.com>
Cc: linux-input(a)vger.kernel.org
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
---
drivers/input/mouse/synaptics.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-next-20200327.orig/drivers/input/mouse/synaptics.c
+++ linux-next-20200327/drivers/input/mouse/synaptics.c
@@ -20,6 +20,7 @@
* Trademarks are the property of their respective owners.
*/
+#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/delay.h>
#include <linux/dmi.h>
@@ -1103,7 +1104,7 @@ static void synaptics_process_packet(str
break;
case 2:
if (SYN_MODEL_PEN(info->model_id))
- ; /* Nothing, treat a pen as a single finger */
+ do_empty(); /* Nothing, treat a pen as a single finger */
break;
case 4 ... 15:
if (SYN_CAP_PALMDETECT(info->capabilities))
4:41 a.m.
New subject: [PATCH 4/9] sound: fix empty-body warning in vx_core.c
Fix gcc empty-body warning when -Wextra is used:
../sound/drivers/vx/vx_core.c:515:3: warning: suggest braces around empty body in an ‘if’
statement [-Wempty-body]
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Jaroslav Kysela <perex(a)perex.cz>
Cc: Takashi Iwai <tiwai(a)suse.com>
Cc: alsa-devel(a)alsa-project.org
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
---
sound/drivers/vx/vx_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-next-20200327.orig/sound/drivers/vx/vx_core.c
+++ linux-next-20200327/sound/drivers/vx/vx_core.c
@@ -13,6 +13,7 @@
#include <linux/init.h>
#include <linux/device.h>
#include <linux/firmware.h>
+#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/io.h>
#include <sound/core.h>
@@ -512,7 +513,7 @@ irqreturn_t snd_vx_threaded_irq_handler(
* received by the board is equal to one of those given to it).
*/
if (events & TIME_CODE_EVENT_PENDING)
- ; /* so far, nothing to do yet */
+ do_empty(); /* so far, nothing to do yet */
/* The frequency has changed on the board (UER mode). */
if (events & FREQUENCY_CHANGE_EVENT_PENDING)
4:41 a.m.
New subject: [PATCH 5/9] usb: fix empty-body warning in sysfs.c
Fix gcc empty-body warning when -Wextra is used:
../drivers/usb/core/sysfs.c: In function ‘usb_create_sysfs_intf_files’:
../drivers/usb/core/sysfs.c:1266:3: warning: suggest braces around empty body in an ‘if’
statement [-Wempty-body]
; /* We don't actually care if the function fails. */
^
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Cc: linux-usb(a)vger.kernel.org
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
---
drivers/usb/core/sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-next-20200327.orig/drivers/usb/core/sysfs.c
+++ linux-next-20200327/drivers/usb/core/sysfs.c
@@ -1263,7 +1263,7 @@ void usb_create_sysfs_intf_files(struct
if (!alt->string && !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
alt->string = usb_cache_string(udev, alt->desc.iInterface);
if (alt->string && device_create_file(&intf->dev,
&dev_attr_interface))
- ; /* We don't actually care if the function fails. */
+ do_empty(); /* We don't actually care if the function fails. */
intf->sysfs_files_created = 1;
}
4:41 a.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
Fix gcc empty-body warning when -Wextra is used:
../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty body in an ‘else’
statement [-Wempty-body]
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
Cc: Chuck Lever <chuck.lever(a)oracle.com>
Cc: linux-nfs(a)vger.kernel.org
---
fs/nfsd/nfs4state.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-next-20200417.orig/fs/nfsd/nfs4state.c
+++ linux-next-20200417/fs/nfsd/nfs4state.c
@@ -34,6 +34,7 @@
#include <linux/file.h>
#include <linux/fs.h>
+#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/namei.h>
#include <linux/swap.h>
@@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
copy_clid(new, conf);
gen_confirm(new, nn);
} else /* case 4 (new client) or cases 2, 3 (client reboot): */
- ;
+ do_empty();
new->cl_minorversion = 0;
gen_callback(new, setclid, rqstp);
add_to_unconfirmed(new);
4:41 a.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in devcoredump.c
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(a)infradead.org>
Cc: Johannes Berg <johannes(a)sipsolutions.net>
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)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(a)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);
4:41 a.m.
New subject: [PATCH 8/9] dax: fix empty-body warnings in bus.c
Fix gcc empty-body warning when -Wextra is used:
../drivers/dax/bus.c:93:27: warning: suggest braces around empty body in an ‘else’
statement [-Wempty-body]
../drivers/dax/bus.c:98:29: warning: suggest braces around empty body in an ‘else’
statement [-Wempty-body]
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Dan Williams <dan.j.williams(a)intel.com>
Cc: Vishal Verma <vishal.l.verma(a)intel.com>
Cc: Dave Jiang <dave.jiang(a)intel.com>
Cc: linux-nvdimm(a)lists.01.org
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
---
drivers/dax/bus.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--- linux-next-20200417.orig/drivers/dax/bus.c
+++ linux-next-20200417/drivers/dax/bus.c
@@ -2,6 +2,7 @@
/* Copyright(c) 2017-2018 Intel Corporation. All rights reserved. */
#include <linux/memremap.h>
#include <linux/device.h>
+#include <linux/kernel.h>
#include <linux/mutex.h>
#include <linux/list.h>
#include <linux/slab.h>
@@ -90,12 +91,12 @@ static ssize_t do_id_store(struct device
} else
rc = -ENOMEM;
} else
- /* nothing to remove */;
+ do_empty(); /* nothing to remove */
} else if (action == ID_REMOVE) {
list_del(&dax_id->list);
kfree(dax_id);
} else
- /* dax_id already added */;
+ do_empty(); /* dax_id already added */
mutex_unlock(&dax_bus_lock);
if (rc < 0)
4:41 a.m.
New subject: [PATCH 9/9] target: fix empty-body warning in target_core_pscsi.c
Fix gcc empty-body warning when -Wextra is used:
../drivers/target/target_core_pscsi.c:624:5: warning: suggest braces around empty body in
an ‘if’ statement [-Wempty-body]
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
Cc: "Martin K. Petersen" <martin.petersen(a)oracle.com>
Cc: linux-scsi(a)vger.kernel.org
Cc: target-devel(a)vger.kernel.org
---
drivers/target/target_core_pscsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-next-20200417.orig/drivers/target/target_core_pscsi.c
+++ linux-next-20200417/drivers/target/target_core_pscsi.c
@@ -18,6 +18,7 @@
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/genhd.h>
+#include <linux/kernel.h>
#include <linux/cdrom.h>
#include <linux/ratelimit.h>
#include <linux/module.h>
@@ -621,7 +622,7 @@ static void pscsi_complete_cmd(struct se
buf = transport_kmap_data_sg(cmd);
if (!buf)
- ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */
+ do_empty(); /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */
if (cdb[0] == MODE_SENSE_10) {
if (!(buf[3] & 0x80))
4:44 a.m.
New subject: [PATCH 5/9] usb: fix empty-body warning in sysfs.c
On Sat, Apr 18, 2020 at 11:41:07AM -0700, Randy Dunlap wrote:
+++ linux-next-20200327/drivers/usb/core/sysfs.c
@@ -1263,7 +1263,7 @@ void usb_create_sysfs_intf_files(struct
if (!alt->string && !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
alt->string = usb_cache_string(udev, alt->desc.iInterface);
if (alt->string && device_create_file(&intf->dev,
&dev_attr_interface))
- ; /* We don't actually care if the function fails. */
+ do_empty(); /* We don't actually care if the function fails. */
intf->sysfs_files_created = 1;
}
Why not just?
+ if (alt->string)
+ device_create_file(&intf->dev, &dev_attr_interface);
4:44 a.m.
New subject: [RFC PATCH 1/9] kernel.h: add do_empty() macro
On Sat, 2020-04-18 at 11:41 -0700, Randy Dunlap wrote:
Add the do_empty() macro to silence gcc warnings about an empty body
following an "if" statement when -Wextra is used.
However, for debug printk calls that are being disabled, use either
no_printk() or pr_debug() [and optionally dynamic printk debugging]
instead.
[]
+#define do_empty() do { } while (0)
If this is really useful
(I think the warning is somewhat silly)
bikeshed:
I think do_nothing() is more descriptive
4:45 a.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
On Apr 18, 2020, at 2:41 PM, Randy Dunlap
<rdunlap(a)infradead.org> wrote:
Fix gcc empty-body warning when -Wextra is used:
../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty body in an ‘else’
statement [-Wempty-body]
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
Cc: Chuck Lever <chuck.lever(a)oracle.com>
Cc: linux-nfs(a)vger.kernel.org
I have a patch in my queue that addresses this particular warning,
but your change works for me too.
Acked-by: Chuck Lever <chuck.lever(a)oracle.com>
Unless Bruce objects.
---
fs/nfsd/nfs4state.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-next-20200417.orig/fs/nfsd/nfs4state.c
+++ linux-next-20200417/fs/nfsd/nfs4state.c
@@ -34,6 +34,7 @@
#include <linux/file.h>
#include <linux/fs.h>
+#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/namei.h>
#include <linux/swap.h>
@@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
copy_clid(new, conf);
gen_confirm(new, nn);
} else /* case 4 (new client) or cases 2, 3 (client reboot): */
- ;
+ do_empty();
new->cl_minorversion = 0;
gen_callback(new, setclid, rqstp);
add_to_unconfirmed(new);
--
Chuck Lever
4:46 a.m.
New subject: [PATCH 5/9] usb: fix empty-body warning in sysfs.c
On 4/18/20 11:44 AM, Matthew Wilcox wrote:
On Sat, Apr 18, 2020 at 11:41:07AM -0700, Randy Dunlap wrote:
> +++ linux-next-20200327/drivers/usb/core/sysfs.c
> @@ -1263,7 +1263,7 @@ void usb_create_sysfs_intf_files(struct
> if (!alt->string && !(udev->quirks &
USB_QUIRK_CONFIG_INTF_STRINGS))
> alt->string = usb_cache_string(udev, alt->desc.iInterface);
> if (alt->string && device_create_file(&intf->dev,
&dev_attr_interface))
> - ; /* We don't actually care if the function fails. */
> + do_empty(); /* We don't actually care if the function fails. */
> intf->sysfs_files_created = 1;
> }
Why not just?
+ if (alt->string)
+ device_create_file(&intf->dev, &dev_attr_interface);
Yes, looks good. Thanks.
--
~Randy
4:49 a.m.
New subject: [RFC PATCH 1/9] kernel.h: add do_empty() macro
On 4/18/20 11:44 AM, Joe Perches wrote:
On Sat, 2020-04-18 at 11:41 -0700, Randy Dunlap wrote:
> Add the do_empty() macro to silence gcc warnings about an empty body
> following an "if" statement when -Wextra is used.
>
> However, for debug printk calls that are being disabled, use either
> no_printk() or pr_debug() [and optionally dynamic printk debugging]
> instead.
[]
> +#define do_empty() do { } while (0)
If this is really useful
(I think the warning is somewhat silly)
bikeshed:
I think do_nothing() is more descriptive
Yes, I do too, as I more or less mentioned in the cover letter.
--
~Randy
4:50 a.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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");
4:53 a.m.
New subject: [PATCH 2/9] fs: fix empty-body warning in posix_acl.c
On Sat, Apr 18, 2020 at 11:41 AM Randy Dunlap <rdunlap(a)infradead.org> wrote:
Fix gcc empty-body warning when -Wextra is used:
Please don't do this.
First off, "do_empty()" adds nothing but confusion. Now it
syntactically looks like it does something, and it's a new pattern to
everybody. I've never seen it before.
Secondly, even if we were to do this, then the patch would be wrong:
if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED)
- /* fall through */ ;
+ do_empty(); /* fall through */
That comment made little sense before, but it makes _no_ sense now.
What fall-through? I'm guessing it meant to say "nothing", and
somebody was confused. With "do_empty()", it's even more confusing.
Thirdly, there's a *reason* why "-Wextra" isn't used.
The warnings enabled by -Wextra are usually complete garbage, and
trying to fix them often makes the code worse. Exactly like here.
Linus
4:53 a.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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.
--
~Randy
4:53 a.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
On Sat, 2020-04-18 at 14:45 -0400, Chuck Lever wrote:
> On Apr 18, 2020, at 2:41 PM, Randy Dunlap
<rdunlap(a)infradead.org> wrote:
>
> Fix gcc empty-body warning when -Wextra is used:
>
> ../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty body in an
‘else’ statement [-Wempty-body]
>
> Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
> Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
> Cc: Andrew Morton <akpm(a)linux-foundation.org>
> Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
> Cc: Chuck Lever <chuck.lever(a)oracle.com>
> Cc: linux-nfs(a)vger.kernel.org
I have a patch in my queue that addresses this particular warning,
but your change works for me too.
Acked-by: Chuck Lever <chuck.lever(a)oracle.com>
Unless Bruce objects.
> ---
> fs/nfsd/nfs4state.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- linux-next-20200417.orig/fs/nfsd/nfs4state.c
> +++ linux-next-20200417/fs/nfsd/nfs4state.c
> @@ -34,6 +34,7 @@
>
> #include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/namei.h>
> #include <linux/swap.h>
> @@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
> copy_clid(new, conf);
> gen_confirm(new, nn);
> } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> - ;
> + do_empty();
> new->cl_minorversion = 0;
> gen_callback(new, setclid, rqstp);
> add_to_unconfirmed(new);
This empty else seems silly and could likely be better handled by
a comment above the first if, something like:
/* for now only handle case 1: probable callback update */
if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
copy_clid(new, conf);
gen_confirm(new, nn);
}
with no else use.
4:55 a.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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
4:55 a.m.
New subject: [PATCH 2/9] fs: fix empty-body warning in posix_acl.c
On 4/18/20 11:53 AM, Linus Torvalds wrote:
On Sat, Apr 18, 2020 at 11:41 AM Randy Dunlap
<rdunlap(a)infradead.org> wrote:
>
> Fix gcc empty-body warning when -Wextra is used:
Please don't do this.
First off, "do_empty()" adds nothing but confusion. Now it
syntactically looks like it does something, and it's a new pattern to
everybody. I've never seen it before.
Secondly, even if we were to do this, then the patch would be wrong:
> if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED)
> - /* fall through */ ;
> + do_empty(); /* fall through */
That comment made little sense before, but it makes _no_ sense now.
What fall-through? I'm guessing it meant to say "nothing", and
somebody was confused. With "do_empty()", it's even more confusing.
Thirdly, there's a *reason* why "-Wextra" isn't used.
The warnings enabled by -Wextra are usually complete garbage, and
trying to fix them often makes the code worse. Exactly like here.
OK, no problem. That's why PATCH 0/9 says RFC.
Oops. Crap. It was *supposed* to say RFC. :(
--
~Randy
4:57 a.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
On 4/18/20 11:53 AM, Joe Perches wrote:
On Sat, 2020-04-18 at 14:45 -0400, Chuck Lever wrote:
>> On Apr 18, 2020, at 2:41 PM, Randy Dunlap <rdunlap(a)infradead.org> wrote:
>>
>> Fix gcc empty-body warning when -Wextra is used:
>>
>> ../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty body in an
‘else’ statement [-Wempty-body]
>>
>> Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
>> Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
>> Cc: Andrew Morton <akpm(a)linux-foundation.org>
>> Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
>> Cc: Chuck Lever <chuck.lever(a)oracle.com>
>> Cc: linux-nfs(a)vger.kernel.org
>
> I have a patch in my queue that addresses this particular warning,
> but your change works for me too.
>
> Acked-by: Chuck Lever <chuck.lever(a)oracle.com>
>
> Unless Bruce objects.
>
>
>> ---
>> fs/nfsd/nfs4state.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> --- linux-next-20200417.orig/fs/nfsd/nfs4state.c
>> +++ linux-next-20200417/fs/nfsd/nfs4state.c
>> @@ -34,6 +34,7 @@
>>
>> #include <linux/file.h>
>> #include <linux/fs.h>
>> +#include <linux/kernel.h>
>> #include <linux/slab.h>
>> #include <linux/namei.h>
>> #include <linux/swap.h>
>> @@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
>> copy_clid(new, conf);
>> gen_confirm(new, nn);
>> } else /* case 4 (new client) or cases 2, 3 (client reboot): */
>> - ;
>> + do_empty();
>> new->cl_minorversion = 0;
>> gen_callback(new, setclid, rqstp);
>> add_to_unconfirmed(new);
This empty else seems silly and could likely be better handled by
a comment above the first if, something like:
/* for now only handle case 1: probable callback update */
if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
copy_clid(new, conf);
gen_confirm(new, nn);
}
with no else use.
I'll just let Chuck handle it with his current patch,
whatever it is.
thanks.
--
~Randy
5:13 a.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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.
5:15 a.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in devcoredump.c
On Sat, Apr 18, 2020 at 11:57 AM Joe Perches <joe(a)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
5:16 a.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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
5:54 a.m.
New subject: [PATCH 5/9] usb: fix empty-body warning in sysfs.c
On Sat, 18 Apr 2020, Matthew Wilcox wrote:
On Sat, Apr 18, 2020 at 11:41:07AM -0700, Randy Dunlap wrote:
> +++ linux-next-20200327/drivers/usb/core/sysfs.c
> @@ -1263,7 +1263,7 @@ void usb_create_sysfs_intf_files(struct
> if (!alt->string && !(udev->quirks &
USB_QUIRK_CONFIG_INTF_STRINGS))
> alt->string = usb_cache_string(udev, alt->desc.iInterface);
> if (alt->string && device_create_file(&intf->dev,
&dev_attr_interface))
> - ; /* We don't actually care if the function fails. */
> + do_empty(); /* We don't actually care if the function fails. */
> intf->sysfs_files_created = 1;
> }
Why not just?
+ if (alt->string)
+ device_create_file(&intf->dev, &dev_attr_interface);
This is another __must_check function call.
The reason we don't care if the call fails is because the file
being created holds the USB interface string descriptor, something
which is purely informational and hardly ever gets set (and no doubt
gets used even less often).
Is this another situation where the comment should be expanded and the
code modified to include a useless test and cast-to-void?
Or should device_create_file() not be __must_check after all?
Greg?
Alan Stern
8:20 a.m.
New subject: [RFC PATCH 1/9] kernel.h: add do_empty() macro
On 4/18/20 11:41 AM, Randy Dunlap wrote:
--- linux-next-20200327.orig/include/linux/kernel.h
+++ linux-next-20200327/include/linux/kernel.h
@@ -40,6 +40,14 @@
#define READ 0
#define WRITE 1
+/*
+ * When using -Wextra, an "if" statement followed by an empty block
+ * (containing only a ';'), produces a warning from gcc:
+ * warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body]
+ * Replace the empty body with do_empty() to silence this warning.
+ */
+#define do_empty() do { } while (0)
+
/**
* ARRAY_SIZE - get the number of elements in array @arr
* @arr: array to be sized
I'm less than enthusiast about introducing a new macro to suppress
"empty body" warnings. Anyone who encounters code in which this macro is
used will have to look up the definition of this macro to learn what it
does. Has it been considered to suppress empty body warnings by changing
the empty bodies from ";" into "{}"?
Thanks,
Bart.
8:24 a.m.
New subject: [RFC PATCH 1/9] kernel.h: add do_empty() macro
On 4/18/20 3:20 PM, Bart Van Assche wrote:
On 4/18/20 11:41 AM, Randy Dunlap wrote:
> --- linux-next-20200327.orig/include/linux/kernel.h
> +++ linux-next-20200327/include/linux/kernel.h
> @@ -40,6 +40,14 @@
> #define READ 0
> #define WRITE 1
> +/*
> + * When using -Wextra, an "if" statement followed by an empty block
> + * (containing only a ';'), produces a warning from gcc:
> + * warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body]
> + * Replace the empty body with do_empty() to silence this warning.
> + */
> +#define do_empty() do { } while (0)
> +
> /**
> * ARRAY_SIZE - get the number of elements in array @arr
> * @arr: array to be sized
I'm less than enthusiast about introducing a new macro to suppress "empty
body" warnings. Anyone who encounters code in which this macro is used will have to
look up the definition of this macro to learn what it does. Has it been considered to
suppress empty body warnings by changing the empty bodies from ";" into
"{}"?
I mentioned that possibility in PATCH 0/9 (cover letter)...
which should have been RFC PATCH 0/9.
So yes, it is possible.
You are the only other person who has mentioned it.
thanks.
--
~Randy
8:28 a.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
On Sat, 2020-04-18 at 14:45 -0400, Chuck Lever wrote:
> On Apr 18, 2020, at 2:41 PM, Randy Dunlap
<rdunlap(a)infradead.org>
> wrote:
>
> Fix gcc empty-body warning when -Wextra is used:
>
> ../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty
> body in an ‘else’ statement [-Wempty-body]
>
> Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
> Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
> Cc: Andrew Morton <akpm(a)linux-foundation.org>
> Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
> Cc: Chuck Lever <chuck.lever(a)oracle.com>
> Cc: linux-nfs(a)vger.kernel.org
I have a patch in my queue that addresses this particular warning,
but your change works for me too.
Acked-by: Chuck Lever <chuck.lever(a)oracle.com>
Unless Bruce objects.
> ---
> fs/nfsd/nfs4state.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- linux-next-20200417.orig/fs/nfsd/nfs4state.c
> +++ linux-next-20200417/fs/nfsd/nfs4state.c
> @@ -34,6 +34,7 @@
>
> #include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/namei.h>
> #include <linux/swap.h>
> @@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
> copy_clid(new, conf);
> gen_confirm(new, nn);
> } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> - ;
> + do_empty();
Urgh... This is just for documentation purposes anyway, so why not just
turn it all into a comment by moving the 'else' into the comment field?
i.e.
} /* else case 4 (.... */
new->cl_minorversion = 0;
> gen_callback(new, setclid, rqstp);
> add_to_unconfirmed(new);
--
Chuck Lever
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust(a)hammerspace.com
8:32 a.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
On 4/18/20 3:28 PM, Trond Myklebust wrote:
On Sat, 2020-04-18 at 14:45 -0400, Chuck Lever wrote:
>> On Apr 18, 2020, at 2:41 PM, Randy Dunlap <rdunlap(a)infradead.org>
>> wrote:
>>
>> Fix gcc empty-body warning when -Wextra is used:
>>
>> ../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty
>> body in an ‘else’ statement [-Wempty-body]
>>
>> Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
>> Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
>> Cc: Andrew Morton <akpm(a)linux-foundation.org>
>> Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
>> Cc: Chuck Lever <chuck.lever(a)oracle.com>
>> Cc: linux-nfs(a)vger.kernel.org
>
> I have a patch in my queue that addresses this particular warning,
> but your change works for me too.
>
> Acked-by: Chuck Lever <chuck.lever(a)oracle.com>
>
> Unless Bruce objects.
>
>
>> ---
>> fs/nfsd/nfs4state.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> --- linux-next-20200417.orig/fs/nfsd/nfs4state.c
>> +++ linux-next-20200417/fs/nfsd/nfs4state.c
>> @@ -34,6 +34,7 @@
>>
>> #include <linux/file.h>
>> #include <linux/fs.h>
>> +#include <linux/kernel.h>
>> #include <linux/slab.h>
>> #include <linux/namei.h>
>> #include <linux/swap.h>
>> @@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
>> copy_clid(new, conf);
>> gen_confirm(new, nn);
>> } else /* case 4 (new client) or cases 2, 3 (client reboot): */
>> - ;
>> + do_empty();
Urgh... This is just for documentation purposes anyway, so why not just
turn it all into a comment by moving the 'else' into the comment field?
i.e.
} /* else case 4 (.... */
new->cl_minorversion = 0;
>> gen_callback(new, setclid, rqstp);
>> add_to_unconfirmed(new);
Like I said earlier, since Chuck has a patch that addresses this,
let's just go with that.
thanks.
--
~Randy
8:33 a.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
On Apr 18, 2020, at 6:32 PM, Randy Dunlap
<rdunlap(a)infradead.org> wrote:
On 4/18/20 3:28 PM, Trond Myklebust wrote:
> On Sat, 2020-04-18 at 14:45 -0400, Chuck Lever wrote:
>>> On Apr 18, 2020, at 2:41 PM, Randy Dunlap <rdunlap(a)infradead.org>
>>> wrote:
>>>
>>> Fix gcc empty-body warning when -Wextra is used:
>>>
>>> ../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty
>>> body in an ‘else’ statement [-Wempty-body]
>>>
>>> Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
>>> Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
>>> Cc: Andrew Morton <akpm(a)linux-foundation.org>
>>> Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
>>> Cc: Chuck Lever <chuck.lever(a)oracle.com>
>>> Cc: linux-nfs(a)vger.kernel.org
>>
>> I have a patch in my queue that addresses this particular warning,
>> but your change works for me too.
>>
>> Acked-by: Chuck Lever <chuck.lever(a)oracle.com>
>>
>> Unless Bruce objects.
>>
>>
>>> ---
>>> fs/nfsd/nfs4state.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> --- linux-next-20200417.orig/fs/nfsd/nfs4state.c
>>> +++ linux-next-20200417/fs/nfsd/nfs4state.c
>>> @@ -34,6 +34,7 @@
>>>
>>> #include <linux/file.h>
>>> #include <linux/fs.h>
>>> +#include <linux/kernel.h>
>>> #include <linux/slab.h>
>>> #include <linux/namei.h>
>>> #include <linux/swap.h>
>>> @@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
>>> copy_clid(new, conf);
>>> gen_confirm(new, nn);
>>> } else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>> - ;
>>> + do_empty();
>
> Urgh... This is just for documentation purposes anyway, so why not just
> turn it all into a comment by moving the 'else' into the comment field?
>
> i.e.
> } /* else case 4 (.... */
>
> new->cl_minorversion = 0;
>>> gen_callback(new, setclid, rqstp);
>>> add_to_unconfirmed(new);
Like I said earlier, since Chuck has a patch that addresses this,
let's just go with that.
I'll post that patch for review as part of my NFSD for-5.8 patches.
--
Chuck Lever
4:02 p.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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(a)infradead.org>
Cc: Johannes Berg <johannes(a)sipsolutions.net>
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)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(a)sipsolutions.net>
*/
+#include <linux/kernel.h>
Why the need for this .h file being added for reformatting the code?
thanks,
greg k-h
4:04 p.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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(a)infradead.org>
> Cc: Johannes Berg <johannes(a)sipsolutions.net>
> Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
> Cc: Andrew Morton <akpm(a)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(a)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...
6:15 p.m.
New subject: [PATCH 8/9] dax: fix empty-body warnings in bus.c
On Sat, Apr 18, 2020 at 11:41:10AM -0700, Randy Dunlap wrote:
rc = -ENOMEM;
} else
- /* nothing to remove */;
+ do_empty(); /* nothing to remove */
} else if (action == ID_REMOVE) {
list_del(&dax_id->list);
kfree(dax_id);
} else
- /* dax_id already added */;
+ do_empty(); /* dax_id already added */
This is just nasty. Please just always turn this bogus warning off
as the existing code is a perfectly readable idiom while the new code
is just nasty crap for no good reason at all.
7:32 p.m.
New subject: [PATCH 6/9] nfsd: fix empty-body warning in nfs4state.c
Hello!
On 18.04.2020 21:41, Randy Dunlap wrote:
Fix gcc empty-body warning when -Wextra is used:
../fs/nfsd/nfs4state.c:3898:3: warning: suggest braces around empty body in an ‘else’
statement [-Wempty-body]
Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org>
Cc: Linus Torvalds <torvalds(a)linux-foundation.org>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
Cc: "J. Bruce Fields" <bfields(a)fieldses.org>
Cc: Chuck Lever <chuck.lever(a)oracle.com>
Cc: linux-nfs(a)vger.kernel.org
---
fs/nfsd/nfs4state.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-next-20200417.orig/fs/nfsd/nfs4state.c
+++ linux-next-20200417/fs/nfsd/nfs4state.c
[...]
@@ -3895,7 +3896,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp
copy_clid(new, conf);
gen_confirm(new, nn);
} else /* case 4 (new client) or cases 2, 3 (client reboot): */
- ;
+ do_empty();
In this case explicit {} would probably have been better, as described in
Documentation/process/coding-style.rst, clause (3).
MBR, Sergei
10:03 p.m.
New subject: [PATCH 7/9] drivers/base: fix empty-body warnings in
devcoredump.c
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(a)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
5:58 a.m.
New subject: [PATCH 2/9] fix empty-body warning in posix_acl.c
On Apr 18, 2020, at 1:53 PM, Linus Torvalds
<torvalds(a)linux-foundation.org> wrote:
Thirdly, there's a *reason* why "-Wextra" isn't used.
The warnings enabled by -Wextra are usually complete garbage, and
trying to fix them often makes the code worse. Exactly like here.
As the instigator of this warning cleanup activity, even _I_ don’t recommend
building with all of -Wextra. Doing so on an allmodconfig build generates
500 megabytes of warning text (not exaggerating), primarily due to
-Wunused-parameter and Wmissing-field-initializers.
I strongly recommend disabling them with -Wno-unused-parameter
-Wno-missing-field-initializers since the spew is completely unactionable.
On the other hand, -Woverride-init found a legit bug that was breaking DVD
drives, fixed in commit 03264ddde2453f6877a7d637d84068079632a3c5.
I believe finding a good set of compiler warning settings can lead to lots of
good bugs being spotted and (hopefully) fixed. Why let syzbot do all the work?
zzy
11:20 a.m.
New subject: [PATCH 5/9] usb: fix empty-body warning in sysfs.c
On Sat, Apr 18 2020, Alan Stern wrote:
On Sat, 18 Apr 2020, Matthew Wilcox wrote:
> On Sat, Apr 18, 2020 at 11:41:07AM -0700, Randy Dunlap wrote:
> > +++ linux-next-20200327/drivers/usb/core/sysfs.c
> > @@ -1263,7 +1263,7 @@ void usb_create_sysfs_intf_files(struct
> > if (!alt->string && !(udev->quirks &
USB_QUIRK_CONFIG_INTF_STRINGS))
> > alt->string = usb_cache_string(udev, alt->desc.iInterface);
> > if (alt->string && device_create_file(&intf->dev,
&dev_attr_interface))
> > - ; /* We don't actually care if the function fails. */
> > + do_empty(); /* We don't actually care if the function fails. */
> > intf->sysfs_files_created = 1;
> > }
>
> Why not just?
>
> + if (alt->string)
> + device_create_file(&intf->dev, &dev_attr_interface);
This is another __must_check function call.
The reason we don't care if the call fails is because the file
being created holds the USB interface string descriptor, something
which is purely informational and hardly ever gets set (and no doubt
gets used even less often).
Is this another situation where the comment should be expanded and the
code modified to include a useless test and cast-to-void?
Or should device_create_file() not be __must_check after all?
One approach to dealing with __must_check function that you don't want
to check is to cause failure to call
pr_debug("usb: interface descriptor file not created");
or similar. It silences the compiler, serves as documentation, and
creates a message that is almost certainly never seen.
This is what I did in drivers/md/md.c...
if (mddev->kobj.sd &&
sysfs_create_group(&mddev->kobj, &md_bitmap_group))
pr_debug("pointless warning\n");
(I give better warnings elsewhere - I must have run out of patience by
this point).
NeilBrown
11:58 p.m.
New subject: [PATCH 5/9] usb: fix empty-body warning in sysfs.c
On Tue, 21 Apr 2020, NeilBrown wrote:
On Sat, Apr 18 2020, Alan Stern wrote:
> On Sat, 18 Apr 2020, Matthew Wilcox wrote:
>
>> On Sat, Apr 18, 2020 at 11:41:07AM -0700, Randy Dunlap wrote:
>> > +++ linux-next-20200327/drivers/usb/core/sysfs.c
>> > @@ -1263,7 +1263,7 @@ void usb_create_sysfs_intf_files(struct
>> > if (!alt->string && !(udev->quirks &
USB_QUIRK_CONFIG_INTF_STRINGS))
>> > alt->string = usb_cache_string(udev, alt->desc.iInterface);
>> > if (alt->string && device_create_file(&intf->dev,
&dev_attr_interface))
>> > - ; /* We don't actually care if the function fails. */
>> > + do_empty(); /* We don't actually care if the function fails. */
>> > intf->sysfs_files_created = 1;
>> > }
>>
>> Why not just?
>>
>> + if (alt->string)
>> + device_create_file(&intf->dev, &dev_attr_interface);
>
> This is another __must_check function call.
>
> The reason we don't care if the call fails is because the file
> being created holds the USB interface string descriptor, something
> which is purely informational and hardly ever gets set (and no doubt
> gets used even less often).
>
> Is this another situation where the comment should be expanded and the
> code modified to include a useless test and cast-to-void?
>
> Or should device_create_file() not be __must_check after all?
One approach to dealing with __must_check function that you don't want
to check is to cause failure to call
pr_debug("usb: interface descriptor file not created");
or similar. It silences the compiler, serves as documentation, and
creates a message that is almost certainly never seen.
This is what I did in drivers/md/md.c...
if (mddev->kobj.sd &&
sysfs_create_group(&mddev->kobj, &md_bitmap_group))
pr_debug("pointless warning\n");
(I give better warnings elsewhere - I must have run out of patience by
this point).
That's a decent idea. I'll do something along those lines.
Alan Stern
880
days inactive
883
days old
38 comments
14 participants
participants (14)
-
Alan Stern -
Bart Van Assche -
Christoph Hellwig -
Chuck Lever -
Greg Kroah-Hartman -
Joe Perches -
Johannes Berg -
Linus Torvalds -
Matthew Wilcox -
NeilBrown -
Randy Dunlap -
Sergei Shtylyov -
Trond Myklebust -
Zzy Wysm