Date:	Sun, 29 Sep 2013 10:50:50 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Tejun Heo <tj@...nel.org>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info'
 in all related extern functions

On 09/27/2013 08:19 PM, Chen Gang wrote:
> On 09/27/2013 07:53 PM, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Sep 27, 2013 at 03:16:59PM +0800, Chen Gang wrote:
>>> Hmm... do you mean: "can not evaluate an interface before implement(or
>>> read details) them all"?
>>
>> No, I'm saying there are a lot more steps necessary between
>> recognizing that an interface needs an improvement and actually
>> improving it than what you're doing now.
>>
>>> If we are agree with each other that "this interface can be improved",
>>> I will go ahead:
>>>
>>>   I will reference the information which Paul McKenney provided.
>>>   And also, I will use LTP's some features to give a test.
>>>   And also, I will reference some contents you said above.
>>>
>>>   Hope I can finish within next month (2013-10-31).
>>
>> If you want to, go ahead but please see below.
>>
>>>> So, please take some time to mull over why your initial patch was
>>>> completely wrong and I didn't even have to read the code to predict
>>>> that your patch has high chance of being wrong.  Now, you're doing the
>>>> *exactly* same thing in the opposite direction.  You should be able to
>>>> recognize that there's something very wrong with that.
>>>
>>> No, I don't think so, in my opinion, for evaluate an api interface,
>>> don't need see the details implementation, even don't need know all
>>> demands.
>>>
>>> During discussing, anyone can make mistakes, in fact, that is the main
>>> reason why we need discussing.
>>>
>>> Hmm... in my opinion, for evaluate one's way/method whether suitable or
>>> not, it is not based on 1-2 mistakes, it need based on mistake/correct ratio.
>>
>> The thing is you are showing a classical and common failure pattern
>> which is known to lead to bad code.  The only safe thing you'd be able
>> to do with your current pattern is making changes which are completely
>> contained and don't affect its interaction with large body of code,
>> and by not doing the necessary steps, you're shifting what you should
>> have done to your reviewers.
>>
>> Your patch is bascially just saying "this part looks a bit
>> inconsistent and may need to be improved" and that's all it is.  This
>> is bad in two ways.  Firstly, the workload on reviewer is higher as
>> they have to do the actual work.  Secondly, it's a lot more likely to
>> lead to bugs as the developer is supposed to be our first and best
>> line of defense against introducing silliness and reviewers operate on
>> the assumption that the developer did her role.
>>
>> Please recognize that obvious local changes and changes which may
>> affect larger interaction are different.  You will need to either
>> stick to obvious local changes or put a lot of effort into learning
>> how to do larger scope work.
>>
> 
> Do we agree with each other:
> 
>   Current 'groups' interface need be improved, although maybe my 2 fix patches are incorrect (but also maybe one of them is correct).
>   And we need additional steps to find the correct fix.
> 
> If so, I should continue, or I think we still need discussing.
> 
> 
>> I hope you understand what I mean.  If not, I don't know what else I
>> can do.  I already spent too much time on this thread and probably
>> won't be as verbose in my future interactions, so if you can come up
>> with a good patch with convincing enough presentation, go for it.  If
>> not, I'm likely to nack it again.
>>
> 
> Hmm... I can understand your feelings. :-)
> 
>> Thanks.
>>

Hmm... excuse me, before getting agreement, I can not "go an inch". And
I think it is still valuable to discuss about it before "go an inch".

How about use WARN_ON() on "!group_info" for groups_search()? It can
let 'groups' interface 'explains' itself 'reasonably' (maybe the 3rd
'patch' is just bothering you? ;-) ).


-------------------------------patch begin------------------------------

kernel/groups.c: add WARN_ON() on "!group_info" for groups_search()

  'groups' interface assumes caller need be sure of 'group_info' valid
  (if 'group_info' is not allocated, it need point to 'init_group').

  If callers pass invalid 'group_info' to groups_search(), we can sure
  "current usage is incorrect, and need be fixed", although we can not
  sure "in current condition, OS must be continuing blindly".

  So need add WARN_ON (not BUG_ON) in groups_search() for alerting.


Signed-off-by: Chen Gang <gang.chen@...anux.com>
---
 kernel/groups.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/groups.c b/kernel/groups.c
index 90cf1c3..d201da0 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -136,7 +136,7 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
 {
 	unsigned int left, right;

-	if (!group_info)
+	if (WARN_ON(!group_info))
 		return 0;

 	left = 0;
-- 
1.7.7.6


-------------------------------patch end--------------------------------


> 
> 
> Thanks.
> 


-- 
Chen Gang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists