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.c...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/