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:

quoted

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-u79uwXL29TaiAVqoAR/hOA@public.gmane.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 <redacted>

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