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

Message-ID: <CAHJEyKW8ofQ0fNQ0L4TzqJTEUMDfAPMufGATbX2Ep7JJKcWHQg@mail.gmail.com>
Date:   Thu, 27 Oct 2022 07:12:06 +0100
From:   Tanju Brunostar <tanjubrunostar0@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
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 Wed, Oct 26, 2022 at 2:50 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> 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

I see. thank you for the pointers

Tanju