linux-kernel - Re: [PATCH v4 7/7] block: Assign a lock_class per gendisk used for wait_for

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: <20171025063435.GQ3310@X58A-UD3R>
Date:   Wed, 25 Oct 2017 15:34:35 +0900
From:   Byungchul Park <byungchul.park@....com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     peterz@...radead.org, axboe@...nel.dk, johan@...nel.org,
        tglx@...utronix.de, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, tj@...nel.org, johannes.berg@...el.com,
        oleg@...hat.com, amir73il@...il.com, david@...morbit.com,
        darrick.wong@...cle.com, linux-xfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-block@...r.kernel.org,
        hch@...radead.org, idryomov@...il.com, kernel-team@....com
Subject: Re: [PATCH v4 7/7] block: Assign a lock_class per gendisk used for
 wait_for_completion()

On Wed, Oct 25, 2017 at 08:01:24AM +0200, Ingo Molnar wrote:
> Beyond the #ifdef reduction I mentioned in the other thread, there's four other 
> things I noticed that need to be fixed in this patch:
> 
>  - Please write out 'minor' instead of the 'm' abbreviation that is meaningless. 
>    'm' is only used for trivial wrappers, but this wrapper is not trivial - so 
>    proper canonical variable names should be used.
> 
>  - Since __key and __name is already double underscores that is customary for
>    macros to avoid variable name shadowing, why is 'ret' not such a name?
> 
>  - But, 'ret' is the typical name used for integer returns, not for pointers! 
>    Please check the gendisk code for what the typical name for gendisk pointers
>    is and use that instead of making up new, weird patterns ...
> 
>  - The "(complete)"#minor"("#id")" generated name is pretty bad. Firstly 
>    "complete" is a verb (or adjective), while lock(dep) symbol names should be 
>    nouns! But even "completion" is pretty opaque, how about "gendisk_completion"?
> 
> More careful patches please!

I am sorry for that. I will do my best not to repeat them.

And I re-spined ASAP to make you able to review with the latest version
only including all your suggestions at the previous version.