linux-kernel - Re: [PATCH 04/12] staging: vt6655: Cleanup and rename function MACbSafeSoftwareReset

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]

Date:   Tue, 13 Sep 2022 13:12:55 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Philipp Hortmann <philipp.g.hortmann@...il.com>
Cc:     Forest Bond <forest@...ttletooquiet.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/12] staging: vt6655: Cleanup and rename function
 MACbSafeSoftwareReset

On Sun, Sep 11, 2022 at 12:46:04PM +0200, Philipp Hortmann wrote:
> Rename function MACbSafeSoftwareReset to vt6655_mac_save_soft_reset and
> abyTmpRegData to tmp_reg_data to avoid CamelCase which is not accepted by
> checkpatch.pl. Remove return value bRetVal as it is unused by the calling
> functions.

Please don't mix this kind of stuff into a patch like this.

> Remove unnecessary declaration of function and make function
> static. Change declaration of tmp_reg_data to shorten code.

When I'm reviewing rename patches I have a script where I call
`rename_rev.pl -a` and it just parses the patch and outputs:

RENAMES:
abyTmpRegData => tmp_reg_data
MACbSafeSoftwareReset => vt6655_mac_save_soft_reset

I don't invest much time in thinking about the new names.  The review
takes me about 5 seconds.

Then once you start marking the functions as static then that's slightly
a headache because now I have to look at that part by hand.  But
whatever, you can kind of sell that as one thing.

But then when you mix ignoring the error codes in as well it's a big
headache.  At first I thought it was because MACbSoftwareReset() always
returns true, so I pulled up that code and that's not true.  So then I
am annoyed.  And I pull up the commit message to see what's going on
and sure enough that change is burried in the middle of the paragraph.

Don't do that.  Just do the renames.  Then change all the functions in
one file to static at once.  Then make the function void in a separate
patch.

The other thing is that making functions static is not at all
controversial.  So long as it builds we will always accept those
patches.

Renaming variables is not super controversial.  Sometimes people quarrel
about the new name.  For example, I don't like a tmp variable which has
a long name like "tmp_reg_data".  Just call it either "tmp" or
"reg_data".  Not both.  Don't write an essay.

But making functions void is sort of controversial...  It's probably the
right thing in this context.  But what I'm saying is don't mix things
which different controversial levels, because it means that someone is
going to complain.  I would have ignored the "tmp_reg_data" thing
except now I'm already complaining about stuff I may as well mention it.

regards,
dan carpenter