[PATCH] target: pscsi: avoid Wempty-body warning

[PATCH] target: pscsi: avoid Wempty-body warning

[Arnd Bergmann]

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 */
+			}
 
 			if (cdb[0] == MODE_SENSE_10) {
 				if (!(buf[3] & 0x80))
-- 
2.29.2

Re: [PATCH] target: pscsi: avoid Wempty-body warning

[Christoph Hellwig]

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.

Re: [PATCH] target: pscsi: avoid Wempty-body warning

[Willy Tarreau]

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

Re: [PATCH] target: pscsi: avoid Wempty-body warning

[Arnd Bergmann]

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

Re: [PATCH] target: pscsi: avoid Wempty-body warning

[Willy Tarreau]

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

Re: [PATCH] target: pscsi: avoid Wempty-body warning

[Arnd Bergmann]

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

Re: [PATCH] target: pscsi: avoid Wempty-body warning

[Willy Tarreau]

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

RE: [PATCH] target: pscsi: avoid Wempty-body warning

[David Laight]

..
> 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)

Re: [PATCH] target: pscsi: avoid Wempty-body warning

[Matthew Wilcox]

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.