diff mbox series

[5/9] usb: fix empty-body warning in sysfs.c

Message ID 20200418184111.13401-6-rdunlap@infradead.org (mailing list archive)
State New, archived
Headers show
Series fix -Wempty-body build warnings | expand

Commit Message

Randy Dunlap April 18, 2020, 6:41 p.m. UTC
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@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/usb/core/sysfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Matthew Wilcox (Oracle) April 18, 2020, 6:44 p.m. UTC | #1
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);
Randy Dunlap April 18, 2020, 6:46 p.m. UTC | #2
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.
Alan Stern April 18, 2020, 7:54 p.m. UTC | #3
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
NeilBrown April 21, 2020, 1:20 a.m. UTC | #4
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
Alan Stern April 21, 2020, 1:58 p.m. UTC | #5
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
diff mbox series

Patch

--- 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;
 }