Message-Id: <9A7C762F-D2D4-4248-B18A-5FAB973D22A6@oracle.com>
Date:	Mon, 9 Feb 2009 12:29:30 -0500
From:	Chuck Lever <chuck.lever@...cle.com>
To:	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
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 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.

Not being a maintainer myself, I can't enforce any requirement here.   
I was simply suggesting ways to help the patch make its way  
expediently to reviewers and people who can get it into the kernel.

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.  A single sentence, as Trond suggested,  
would have been far more concise.  And Bruce's recommendations for  
setting off machine-generated content in a patch description are cogent.

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.  A handful of us on linux-nfs  
happen to read netdev, but not all of us.  We do try to stick to a  
fair review process here, which includes archiving the patch and  
reviewer comments via linux-nfs@...r.kernel.org.

J. Bruce Fields continues:
> Anyway, the patch looks obviously fine to me[.]

I don't have an immediate problem with the patch's content either, as  
long as it has been tested and does not introduce any new sparse or  
compiler warnings.  I'm all for the intelligent elimination of gotos  
and duplicated code.

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.

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.

Acked-by: Chuck Lever <chuck.lever@...cle.com>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists