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

Chen Gang
2013-09-27T12:20:40Z

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