linux-kernel - Re: [PATCH 2/2] staging: rtl8192u: Corrects 'Avoid CamelCase' for variables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ee182711405229e85b5b5a44c683d5a2609b5ba3.camel@perches.com>
Date:   Tue, 17 Mar 2020 08:04:26 -0700
From:   Joe Perches <joe@...ches.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Camylla Goncalves Cantanheide <c.cantanheide@...il.com>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, lkcamp@...ts.libreplanetbr.org
Subject: Re: [PATCH 2/2] staging: rtl8192u: Corrects 'Avoid CamelCase' for
 variables

On Tue, 2020-03-17 at 16:43 +0300, Dan Carpenter wrote:
> On Tue, Mar 17, 2020 at 08:51:30AM +0000, Camylla Goncalves Cantanheide wrote:
> > The variables of function setKey triggered a 'Avoid CamelCase'
> > warning from checkpatch.pl. This patch renames these
> > variables to correct this warning.
> > 
> > Signed-off-by: Camylla Goncalves Cantanheide <c.cantanheide@...il.com>
> > ---
> >  drivers/staging/rtl8192u/r8192U_core.c | 52 +++++++++++++-------------
> >  1 file changed, 26 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> > index 93a15d57e..fcfb9024a 100644
> > --- a/drivers/staging/rtl8192u/r8192U_core.c
> > +++ b/drivers/staging/rtl8192u/r8192U_core.c
> > @@ -4877,50 +4877,50 @@ void EnableHWSecurityConfig8192(struct net_device *dev)
> >  	write_nic_byte(dev, SECR,  SECR_value);
> >  }
> >  
> > -void setKey(struct net_device *dev, u8 EntryNo, u8 KeyIndex, u16 KeyType,
> > -	    u8 *MacAddr, u8 DefaultKey, u32 *KeyContent)
> > +void setKey(struct net_device *dev, u8 entryno, u8 keyindex, u16 keytype,
> > +	    u8 *macaddr, u8 defaultkey, u32 *keycontent)
> >  {
> > -	u32 TargetCommand = 0;
> > -	u32 TargetContent = 0;
> > -	u16 usConfig = 0;
> > +	u32 target_command = 0;
> > +	u32 target_content = 0;
> > +	u16 us_config = 0;
> 
> Use these renames to think deeply about naming.
> 
> I don't like "entryno".  I would prefer "entry_no".  Use the same
> underscore for spaces rule for key_index, mac_addr and all the rest.  Is
> "key_idx" better or "key_index"?
> 
> What added value or meaning does the "target_" part of "target_command"
> add?  Use "cmd" instead of "command".  "target_command" and
> "target_content" are the same length and mostly the same letters.  Avoid
> that sort of thing because it makes it hard to read at a glance.  The
> two get swapped in your head.
> 
> What does the "us_" mean in us_config?  Is it microsecond as in usec?
> Is it United states?  Actually it turns out it probably means "unsigned
> short".  Never make the variable names show the type.  If you have a
> good editor you can just hover the mouse over a variable to see the
> type.  Or if you're using vim like me, then you have to use '*' to
> highlight the variable and scroll to the top of the function.  Either
> way, never use "us_" to mean unsigned short.
> 
> What does the "config" part of "us_config" mean?  What does the "content"
> part of "target_content" mean?  Always think about that.  Variable names
> are hard and maybe "config" and "content" are clear enough.  But at
> think about it, and consider all the options.
> 
> Anyway, the reason that this patch needs to be re-written is because
> we want underscores in place of spaces for "key_type" and because
> "us_config" is against the rules.  The rest is just something to
> consider and if you find better names, then go with that but if you
> don't just fix those two things and resend.

What Dan said and:

Make sure to successfully compile the source files the patch
modifies before sending the patch.

Renaming function arguments and not renaming the uses of the
arguments in the function is not good.