linux-kernel - Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up usbctrl

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, 14 Sep 2021 13:18:01 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Michael Straube <straube.linux@...il.com>,
        Pavel Skripkin <paskripkin@...il.com>
Subject: Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up usbctrl_vendorreq()

On Tuesday, September 14, 2021 11:24:05 AM CEST Dan Carpenter wrote:
> On Mon, Sep 13, 2021 at 08:09:59PM +0200, Fabio M. De Francesco wrote:
> > Clean up usbctrl_vendorreq () in usb_ops_linux.c because some
> > of its code will be reused in this series. This cleanup is in
> > preparation for shortening the call chains of rtw_read{8,16,32}()
> > and rtw_write{8,16,32,N}(). More insights about the reasons why at
> > https://lore.kernel.org/lkml/5319192.FrU0QrjFp7@localhost.localdomain/
> > 
> 
> This commit message is quite bad.
> 
> This patch has nothing to do with reusing the code or shortening call
> chains.

It has to do, in a certain sense. Let me explain please...

Some days ago, David Laight made the review of "Shorten calls chain of 
rtw_write8/16/32/n()" version 3. 

In that patch he noticed some lines of usb_read() that I had created with the 
help of reusing some lines of the code of usbctrl_vendorreq() that is deleted 
in the same patch. 

He thought that they were clean-ups and renames and so he suggested to make 
those "clean-ups" in a separate patch.

However they were _not_ renames or other clean-ups, because usb_read() was 
not touched in that patch and, above all, it was a new function. 

I am sure that when I write new functions I can use whatever name of 
variables I like, even if people may think I'm renaming the variables that 
were in a old function that now is deleted. Am I not permitted?

However, because I also think that readability of the diffs matters, I 
decided to do some clean-up of the code I'm about to reuse in the new 
functions. It improves readability of the above-mentioned patch that is also 
the 18/18 of this series.

That is the reason why I'm cleaning up a function that is going to be deleted 
in the last patch of the series.

> Don't use a link like that in the commit message especially when it's a
> link to an email you wrote.  If it's someone else's email you can say,
> something like "As <name> points out in <his/her> email <url>.  Blah
> blah blah."  That way you give credit to the other person but all the
> information is in the commit message.

I agree with you. I'll redo the commit message for in order to summarize in 
few lines why I'm doing clean-ups of functions that must be deleted in 18/18.
The same for 16/18. I think that a short explanation like the one that I gave 
you above should suffice (much shorter, obviously).

I hope that I've been clear now. Please let me know if you have more 
suggestions about this patch and the next (16/18).

Regards,

Fabio