| Message ID | 20210322114441.3479365-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | target: pscsi: avoid Wempty-body warning | expand |
On Mon, Mar 22, 2021 at 12:44:34PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Building with 'make W=1' shows a harmless warning for pscsi: > > drivers/target/target_core_pscsi.c: In function 'pscsi_complete_cmd': > drivers/target/target_core_pscsi.c:624:33: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body] > 624 | ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ > | ^ > > Rework the coding style as suggested by gcc to avoid the warning. I would much, much prefer to drop the bogus warning; if (foo) ; /* comment */ is a fairly usual and absolutely sensible style. The warning on hte other hand is completely stupid.
On Mon, Mar 22, 2021 at 03:47:35PM +0000, Christoph Hellwig wrote: > On Mon, Mar 22, 2021 at 12:44:34PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > Building with 'make W=1' shows a harmless warning for pscsi: > > > > drivers/target/target_core_pscsi.c: In function 'pscsi_complete_cmd': > > drivers/target/target_core_pscsi.c:624:33: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body] > > 624 | ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ > > | ^ > > > > Rework the coding style as suggested by gcc to avoid the warning. > > I would much, much prefer to drop the bogus warning; > > if (foo) > ; /* comment */ > > is a fairly usual and absolutely sensible style. The warning on hte > other hand is completely stupid. Agreed! These days it seems there is a competition for the stupidest warning between compilers, and we've reached the point where working around them manages to introduce real bugs :-( I predict we'll soon see warning such as "this comment looks like valid C code, if you really intended to comment it out, use #if 0 instead". Oh well, let's hope I have not given a new idea here... Willy
On Mon, Mar 22, 2021 at 12:44:34PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Building with 'make W=1' shows a harmless warning for pscsi: > > drivers/target/target_core_pscsi.c: In function 'pscsi_complete_cmd': > drivers/target/target_core_pscsi.c:624:33: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body] > 624 | ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ > | ^ > > Rework the coding style as suggested by gcc to avoid the warning. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/target/target_core_pscsi.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c > index 3cbc074992bc..913b092955f6 100644 > --- a/drivers/target/target_core_pscsi.c > +++ b/drivers/target/target_core_pscsi.c > @@ -620,8 +620,9 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status, > unsigned char *buf; > > buf = transport_kmap_data_sg(cmd); > - if (!buf) > - ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ > + if (!buf) { > + /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ > + } But why not just delete the code? buf = transport_kmap_data_sg(cmd); /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ I mean, this seems like a real warning here. We're not actually handling the failure to allocate 'buf'. It's not marked as __must_check, but watch the code flow: buf = transport_kmap_data_sg(cmd); if (!buf) ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ if (cdb[0] == MODE_SENSE_10) { if (!(buf[3] & 0x80)) buf[3] |= 0x80; } else { if (!(buf[2] & 0x80)) buf[2] |= 0x80; } we're about to do a NULL ptr dereference. So this should be handled somehow.
On Mon, Mar 22, 2021 at 4:53 PM Willy Tarreau <w@1wt.eu> wrote: > On Mon, Mar 22, 2021 at 03:47:35PM +0000, Christoph Hellwig wrote: > > On Mon, Mar 22, 2021 at 12:44:34PM +0100, Arnd Bergmann wrote: > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > Building with 'make W=1' shows a harmless warning for pscsi: > > > > > > drivers/target/target_core_pscsi.c: In function 'pscsi_complete_cmd': > > > drivers/target/target_core_pscsi.c:624:33: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body] > > > 624 | ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ > > > | ^ > > > > > > Rework the coding style as suggested by gcc to avoid the warning. > > > > I would much, much prefer to drop the bogus warning; > > > > if (foo) > > ; /* comment */ > > > > is a fairly usual and absolutely sensible style. The warning on hte > > other hand is completely stupid. > > Agreed! > > These days it seems there is a competition for the stupidest warning > between compilers, and we've reached the point where working around > them manages to introduce real bugs :-( > > I predict we'll soon see warning such as "this comment looks like valid > C code, if you really intended to comment it out, use #if 0 instead". Oh > well, let's hope I have not given a new idea here... I agree that this instance of the warning is particularly stupid, but the I'd like to leave the warning option there and eventually enable it by default because it tends to find other more interesting cases, and this one is trivial to work around. I remember previously fixing a few drivers that did obviously incorrect things like if (error); /* note the extra ';' */ return error; and a lot mostly harmless code like #ifdef DEBUG_THIS_DRIVER /* always disabled */ #define dprintk(args...) printk(args) #else #define dprintk(args...) #endif /* note the mismatched format string */ dprintk(KERN_WARNING "error %d\n", &object); Turning the empty dprintk() into no_printk() means we can catch the wrong format string during compile testing. Arnd
On Mon, Mar 22, 2021 at 05:18:23PM +0100, Arnd Bergmann wrote: > I agree that this instance of the warning is particularly stupid, but the > I'd like to leave the warning option there and eventually enable it by > default because it tends to find other more interesting cases, and this > one is trivial to work around. > > I remember previously fixing a few drivers that did obviously > incorrect things like > > if (error); /* note the extra ';' */ > return error; I totally agree with this one but usually it's already reported by another one (probably the one complaining about misindenting). The case I've seen quite a few times was: while (condition); At least I want the ';' on its own line to avoid it being confused with one that ends a do {} while() block. > and a lot mostly harmless code like > > #ifdef DEBUG_THIS_DRIVER /* always disabled */ > #define dprintk(args...) printk(args) > #else > #define dprintk(args...) > #endif > /* note the mismatched format string */ > dprintk(KERN_WARNING "error %d\n", &object); > > Turning the empty dprintk() into no_printk() means we can catch > the wrong format string during compile testing. Hmmm OK for this one. With this said, given how plenty of warnings seem to consider indent and whatever, I wouldn't be surprised if a difference is made between a totally empty body and one that results from an empty macro. But indeed this one can represent a real bug. Willy
On Mon, Mar 22, 2021 at 5:26 PM Willy Tarreau <w@1wt.eu> wrote: > On Mon, Mar 22, 2021 at 05:18:23PM +0100, Arnd Bergmann wrote: > > and a lot mostly harmless code like > > > > #ifdef DEBUG_THIS_DRIVER /* always disabled */ > > #define dprintk(args...) printk(args) > > #else > > #define dprintk(args...) > > #endif > > /* note the mismatched format string */ > > dprintk(KERN_WARNING "error %d\n", &object); > > > > Turning the empty dprintk() into no_printk() means we can catch > > the wrong format string during compile testing. > > Hmmm OK for this one. With this said, given how plenty of warnings seem > to consider indent and whatever, I wouldn't be surprised if a difference > is made between a totally empty body and one that results from an empty > macro. But indeed this one can represent a real bug. The -Wempty-body warning is actually really old and predates the compiler's understanding of indentation, we just always disabled it by default so far. As a lot of subsystems are W=1 clean these days, I just went for the final 26 patches to shut up all empty-body warnings in randconfig builds. Most of these were in the dprink() category, though none of this last set actually had incorrect format strings. Arnd
On Mon, Mar 22, 2021 at 05:34:48PM +0100, Arnd Bergmann wrote: > On Mon, Mar 22, 2021 at 5:26 PM Willy Tarreau <w@1wt.eu> wrote: > > On Mon, Mar 22, 2021 at 05:18:23PM +0100, Arnd Bergmann wrote: > > > and a lot mostly harmless code like > > > > > > #ifdef DEBUG_THIS_DRIVER /* always disabled */ > > > #define dprintk(args...) printk(args) > > > #else > > > #define dprintk(args...) > > > #endif > > > /* note the mismatched format string */ > > > dprintk(KERN_WARNING "error %d\n", &object); > > > > > > Turning the empty dprintk() into no_printk() means we can catch > > > the wrong format string during compile testing. > > > > Hmmm OK for this one. With this said, given how plenty of warnings seem > > to consider indent and whatever, I wouldn't be surprised if a difference > > is made between a totally empty body and one that results from an empty > > macro. But indeed this one can represent a real bug. > > The -Wempty-body warning is actually really old and predates the compiler's > understanding of indentation, we just always disabled it by default so far. > > As a lot of subsystems are W=1 clean these days, I just went for the > final 26 patches to shut up all empty-body warnings in randconfig builds. > Most of these were in the dprink() category, though none of this last set > actually had incorrect format strings. I agree that if it's only 26 patches on the whole kernel to re-enable one warning it can be worth it for newcomers. Willy
.. > I totally agree with this one but usually it's already reported by > another one (probably the one complaining about misindenting). The > case I've seen quite a few times was: > > while (condition); > > At least I want the ';' on its own line to avoid it being > confused with one that ends a do {} while() block. For that one it is worth doing: while (condition) continue; I've also managed to write: while (...) { .... } while (...); And if anyone thing code should be laid out as: do { ... } while (...); Show them: ... } while (....................................................... { ... and ask them what it is supposed to be. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 3cbc074992bc..913b092955f6 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -620,8 +620,9 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status, unsigned char *buf; buf = transport_kmap_data_sg(cmd); - if (!buf) - ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ + if (!buf) { + /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ + } if (cdb[0] == MODE_SENSE_10) { if (!(buf[3] & 0x80))