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

Chen Gang
2013-09-29T02:51:57Z

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