From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754005Ab3I2Cv5 (ORCPT ); Sat, 28 Sep 2013 22:51:57 -0400 Received: from intranet.asianux.com ([58.214.24.6]:45803 "EHLO intranet.asianux.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753732Ab3I2Cvz (ORCPT ); Sat, 28 Sep 2013 22:51:55 -0400 X-Spam-Score: -100.8 Message-ID: <5247958A.2050805@asianux.com> Date: Sun, 29 Sep 2013 10:50:50 +0800 From: Chen Gang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Tejun Heo CC: Andrew Morton , "linux-kernel@vger.kernel.org" , Michael Kerrisk , Paul McKenney Subject: Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions References: <5242372C.4010509@asianux.com> <20130925011404.GB25777@htj.dyndns.org> <52424094.3020504@asianux.com> <524267CD.4030007@asianux.com> <5243CD18.1060805@asianux.com> <5243D487.8000602@asianux.com> <20130926131544.GB31249@mtj.dyndns.org> <5244DFA5.5090200@asianux.com> <20130927033651.GA9768@htj.dyndns.org> <524530EB.6080109@asianux.com> <20130927115318.GA12206@mtj.dyndns.org> <524577D2.5080308@asianux.com> In-Reply-To: <524577D2.5080308@asianux.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 --- 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