From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6BE13C43334 for ; Wed, 29 Jun 2022 18:09:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230323AbiF2SJG convert rfc822-to-8bit (ORCPT ); Wed, 29 Jun 2022 14:09:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53254 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230073AbiF2SJF (ORCPT ); Wed, 29 Jun 2022 14:09:05 -0400 Received: from relay5.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 10A1211A28 for ; Wed, 29 Jun 2022 11:09:03 -0700 (PDT) Received: from omf10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 826E060C93; Wed, 29 Jun 2022 18:09:02 +0000 (UTC) Received: from [HIDDEN] (Authenticated sender: joe@perches.com) by omf10.hostedemail.com (Postfix) with ESMTPA id 5999F40; Wed, 29 Jun 2022 18:09:01 +0000 (UTC) Message-ID: <2f2d8b6ddb4418bbe351d6c19d08e9241de20087.camel@perches.com> Subject: Re: [PATCH 1/7] Staging: rtl8192e: Added blank lines before/after struct From: Joe Perches To: Felix Schlepper Cc: gregkh@linuxfoundation.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org, wjsota@gmail.com Date: Wed, 29 Jun 2022 11:09:00 -0700 In-Reply-To: References: Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 8BIT User-Agent: Evolution 3.44.1-0ubuntu1 MIME-Version: 1.0 X-Rspamd-Server: rspamout03 X-Rspamd-Queue-Id: 5999F40 X-Stat-Signature: cxpy4bhehp97he7ibrn6fwndwm5kmkk4 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Session-ID: U2FsdGVkX1+PFQLq9iP8uUoYJGt7spzt5iZz7rrAPH0= X-HE-Tag: 1656526141-482888 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > #include > > > #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 > > #include > > #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< > - 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.