Date:	Tue, 10 Feb 2009 15:12:37 +0200 (EET)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	Chuck Lever <chuck.lever@...cle.com>
cc:	"J. Bruce Fields" <bfields@...ldses.org>,
	Trond Myklebust <Trond.Myklebust@...app.com>,
	David Miller <davem@...emloft.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Trond Myklebust <trond@...app.com>,
	Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

On Mon, 9 Feb 2009, Chuck Lever wrote:

> On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote:
> >On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote:
> > >I personally think that people were honestly just mislead to think that
> > >diff-funcs output is part of the patch
> >
> >Yes, exactly, in my case, that's all it was--the "diff" I thought I saw
> >in my 2-second skim looked odd, and I figured a comment might have
> >gotten dropped off in the process of replying and adding cc's--hence the
> >original question.
> 
> I apologize if my request for "English" seemed Anglo-centric.  What I intended
> was "non-machine-generated" and expressed in the natural language that is
> usual for Linux kernel patch descriptions, which appears to be English.

...I still argue that if title gives the same information as that 
sentence would, the sentence is optional. Some patches don't even have 
patch description beyond title and there's very little problem with that. 
As I admitted earlier the title wasn't as good as it should have (and I 
even sent a followup to please all including even the in body sentence 
plus the fixed title). Not that I find the title really a problem to sane 
people who understand that code-duplication is bad and getting rid of it 
is what one should do in general, as long as it's possible without making 
a mess out of it.

> I agree with you that the diff output in the patch description was confusing,
> and yes, after Dave's first comment I understood what the description meant.
> I believe patch authors owe it to maintainers, who have dozens of patches to
> review a day, to keep it as simple and easy as possible.  I have tried to work
> with individual maintainers where possible to improve the legibility of my
> patches, though at times I have fallen short.
> 
> Although interesting, the tool output doesn't seem necessary to justify the
> proposed change.

I don't understand why a tool output cannot "say" the same as some English 
sentence... And if a proof that the code blocks that were merged were 
infact identical (instead of having some hard to spot difference) doesn't 
seem like useful information to reviewer I don't really know what to say.

> In addition to crafting a legible description, my suggestions were about
> cc'ing the proper mailing list, and routing this patch directly to the sunrpc
> kernel maintainers in the first place, all of which are listed within easy
> reach in MAINTAINERS.

Please note that I couldn't guess that I'd have to look for another entry 
after first SUNRPC entry in the MAINTAINERS. So asking outsider to know
who is who in sunrpc realm is not very likely to be a practical 
requirement either. And even to add into confusion the latest commits 
in that particular directory had the last S-o-B line as Bruce's. How could 
I have guessed? Forgetting to lookup the address from MAINTAINERS into the 
first mail is something which just happened and I'm certainly guilty to 
that :-).

...I've had enough down turning experiences with random dev lists listed 
in the MAINTAINERS (mostly spammy subsriber only ones, nearly infinite 
response time, etc things) that I mostly ignore them nowadays though this 
time it would have been vger list so no subscription required madness. 
Instead I either select lkml or netdev, this time netdev because of 
the obvious presence under net/ hierarcy.

> But I wonder how we ended up with these two copies of the same logic.  It may
> be that at some point in the past we had more different logic in the two
> write_space functions, but I suppose we can split it out again if we ever need
> to.  Thinking aloud, at some point we may have lost a little history here, or
> we may have perhaps lost any implicit operational dependencies on other parts
> of xprtsock.c (like the TCP state logic) that are not expressed in the source
> code or comments.

Well, this kind of thinking is beyond me and requires more knowledge about 
internals of the code. IMHO, this kind of speculation changes nothing 
really, and besides the functions _were_ different to begin with as was
proven in the patch description :-). ...Would they have been fully 
identical you would have seen a bit different patch ;-).

If you think there's something wrong, please fix, you're free to do so,
I don't understand enough. But we don't keep all those #if 0 blocks 
around either just because somebody thinks that something will get 
implemented one day (not that you directly made such assertion, probably 
saying mildy the opposite instead). Ironically, you would have been 
perfectly happy with the particular functions unless a cleanup patch 
against them gets made... :-)

> The newly shared code appears to be socket-specific, so it does not 
> belong in the shared function we already have, xprt_write_space(), which 
> is meant to be transport-generic.

IMHO, this is the most useful response so far! :-) Usually the hardest 
obstacles I face with non-familiar code is its structure and naming.


-- 
 i.

Powered by blists - more mailing lists