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@xxxxxxxxxxxxxxxx
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@xxxxxxxxxx>
--
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
