linux-kernel - Re: [PATCH 05/17] staging: vt6655: changed variable name: pvRTS

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y1k7cDP4Dpdr5EOe@kroah.com>
Date:   Wed, 26 Oct 2022 15:51:44 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Tanjuate Brunostar <tanjubrunostar0@...il.com>
Cc:     linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy@...ts.linux.dev
Subject: Re: [PATCH 05/17] staging: vt6655: changed variable name: pvRTS

On Tue, Oct 25, 2022 at 11:37:01PM +0000, Tanjuate Brunostar wrote:

Philipp has pointed out most of this already, but I'll just be specific
and say what isn't ok in all of these patches:

> 	change variable names pvRTS to meet the

"name" not "name"

>         linux coding standard, as it says to avoid using camelCase naming

"Linux" not "linux"

>         style. Cought by checkpatch

Why is this all indented?

Please do not do that, look at existing accepted changes in the git log
and match up what they look like.

But worst of all, you didn't really fix the variable name at all.  You
just appeased a tool that was trying to say "don't use camelCase, use
sane names".

> Signed-off-by: Tanjuate Brunostar <tanjubrunostar0@...il.com>
> ---
>  drivers/staging/vt6655/rxtx.c | 56 +++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> index 2cac8f3882df..e97cba014adf 100644
> --- a/drivers/staging/vt6655/rxtx.c
> +++ b/drivers/staging/vt6655/rxtx.c
> @@ -87,7 +87,7 @@ static const unsigned short w_fb_opt_1[2][5] = {
>  /*---------------------  Static Functions  --------------------------*/
>  static void s_v_fill_rts_head(struct vnt_private *p_device,
>  			      unsigned char by_pkt_type,
> -			      void *pvRTS,
> +			      void *pv_rts,

"pvRTS" is using Hungarian Notation.  Look it up on Wikipedia for what
it means, and why people used to use it.

For us, we don't need that at all as the type of the variable is obvious
in the code and the compiler checks it.

So "pvRTS" is trying to say "this is a pointer to void called "RTS".

We don't care about the "describe the variable type in the name" thing,
so it should just be called "RTS", or better yet, "rts", right?

But then, step back.  Why is this a void pointer at all?  This is really
a structure of type struct vnt_rts_g_fb.  So why isn't that being passed
here instead?

So try to work on both, fixing up the names to be sane, and then,
getting rid of the void * stuff, to better reflect how data is flowing
around and what type that data is in.

thanks,

greg k-h