Thread (11 messages) 11 messages, 2 authors, 2017-10-25

Re: [PATCH v4 7/7] block: Assign a lock_class per gendisk used for wait_for_completion()

From: Byungchul Park <hidden>
Date: 2017-10-25 06:34:35
Also in: linux-fsdevel, linux-mm, linux-xfs, lkml

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help