Re: [PATCH 1/7] Staging: rtl8192e: Added blank lines before/after struct

[Joe Perches]

On Wed, 2022-06-29 at 14:11 +0200, Felix Schlepper wrote:
> On 28.06.2022 10:58, Joe Perches wrote:
> > On Tue, 2022-06-28 at 10:30 +0200, Felix Schlepper wrote:
> > > This addresses an issue raised by checkpatch.pl:
> > > 
> > >      $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
> > >      CHECK: Please use a blank line after function/struct/union/enum
> > >      declarations
> > > 
> > > The additional blank line above the struct/before the headers is
> > > just cleaner.
> > []
> > > diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> > []
> > > @@ -17,10 +17,12 @@
> > >  #include <linux/module.h>
> > >  #include <linux/etherdevice.h>
> > >  #include "rtllib.h"
> > > +
> > >  struct modes_unit {
> > >  	char *mode_string;
> > >  	int mode_size;
> > >  };
> > > +
> > >  static struct modes_unit rtllib_modes[] = {
> > >  	{"a", 1},
> > >  	{"b", 1},
> > 
> > Please always look deeper at the code and do not simply take
> > any checkpatch suggestion as the "best" fix.
> > 
> > Using this struct style with an embedded length is error prone
> > and would likely be better as a simple char * array.
> > 
> > Also, the existing sprintf use of mode_string and mode_size is
> > nonsensical.  There is nothing to format in any of the mode_size.
> > so the use of mode_size as an argument is silly.
> First of all, thank you for your suggestions.
> I did realize that there is nothing to format, but I did not dare to
> make that change in case a format could be added.
> However, looking at it now. This has not been touched in over 10 years.
> So it is probably safe to change that.
> > 
> > Perhaps something like this would be better:
> > ---
> >  drivers/staging/rtl8192e/rtllib_wx.c | 21 +++++----------------
> >  1 file changed, 5 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> > index cf9a240924f20..90dcce152d80c 100644
> > --- a/drivers/staging/rtl8192e/rtllib_wx.c
> > +++ b/drivers/staging/rtl8192e/rtllib_wx.c
> > @@ -17,17 +17,9 @@
> >  #include <linux/module.h>
> >  #include <linux/etherdevice.h>
> >  #include "rtllib.h"
> > -struct modes_unit {
> > -	char *mode_string;
> > -	int mode_size;
> > -};
> > -static struct modes_unit rtllib_modes[] = {
> > -	{"a", 1},
> > -	{"b", 1},
> > -	{"g", 1},
> > -	{"?", 1},
> > -	{"N-24G", 5},
> > -	{"N-5G", 4},
> > +
> > +static const char *rtllib_modes[] = {
> > +	"a", "b", "g", "?", "N-24G", "N-5G",
> >  };
> >  
> >  #define MAX_CUSTOM_LEN 64
> > @@ -72,11 +64,8 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
> >  	/* Add the protocol name */
> >  	iwe.cmd = SIOCGIWNAME;
> >  	for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
> > -		if (network->mode&(1<<i)) {
> > -			sprintf(pname, rtllib_modes[i].mode_string,
> > -				rtllib_modes[i].mode_size);
> > -			pname += rtllib_modes[i].mode_size;
> > -		}
> > +		if (network->mode & BIT(i))
> > +			pname = stpcpy(pname, rtllib_modes[i]);
> >  	}
> >  	*pname = '\0';
> Would this not be also redundant since stpcpy already returns a pointer to the new
> NUL-terminating character?
> >  	snprintf(iwe.u.name, IFNAMSIZ, "IEEE802.11%s", proto_name);

Only if proto_name is initialized.  Is it?

Also I believe stpcpy is not a standard linux call, so maybe
this needs to be strcpy or strncpy then strlen.