From: Paul G. <pau...@wi...> - 2012-10-26 21:36:34
|
[Re: [PATCH net-next v2 1/6] tipc: introduce new TIPC server infrastructure] On 26/10/2012 (Fri 18:07) Ying Xue wrote: > Hi Paul, > > Thank you for your given comments, please see my responses inline. > > Paul Gortmaker wrote: > >[[PATCH net-next v2 1/6] tipc: introduce new TIPC server infrastructure] On 26/09/2012 (Wed 16:20) Ying Xue wrote: > > [...] > >>--- > >> net/tipc/Makefile | 2 +- > >> net/tipc/server.c | 686 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> net/tipc/server.h | 103 ++++++++ > >> 3 files changed, 790 insertions(+), 1 deletions(-) > > > >Looking at the diffstat, it appears to me that the code is added, and it > >is compiled, but since no other files are touched at all, it does > >nothing (i.e. is totally unused) after just this commit is applied. > >For example, tipc_server_start is never called. > > > Yes, in this patch file, we do not call it. > In fact I just want to introduce the new server model in a separate patch. > In subsequent patches I will convert two servers(topology serve and > configuration server). > >The reason I mention this, is because this is somewhat of a forced > >separation. Yes it is good to keep patches smaller where possible, but > >at the same time I have seen people get review comments saying to not > >introduce orphaned code -- i.e. the commit that adds the code should > >also enable and use the code. (If you don't do it this way, then a > >bisect will lead to a random user of broken code, and not the addition > >of the broken code). > > > Do you suggest we should add one conversion(for example, topology > serve) in the patch? Yes -- I think so. Unless it really does make a giant un-reviewable patch out of things. In that case, one might be able to justify it being a stand alone addition of "unused" code, but the commit log would have to actually acknowledge that, with something like: As of this commit, this new server infrastructure is built, but not actually yet called by the existing TIPC code, but since the conversion changes required in order to use it are significant, the addition is kept here as a separate commit. I did a similar thing (but in reverse) when I deleted all the token ring junk. See the 2nd from last paragraph here: http://lwn.net/Articles/497394/ Thanks, Paul. -- |