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);
>
Will resubmit later.
Cheers
Felix Schlepper
