Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions

[Chen Gang <gang.chen@asianux.com>]

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.
> 


Thanks.
-- 
Chen Gang