netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: William Allen Simpson <william.allen.simpson@gmail.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>
Subject: Re: [net-next-2.6 PATCH v5 3/5 RFC] TCPCT part1c: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
Date: Tue, 10 Nov 2009 09:43:28 -0500	[thread overview]
Message-ID: <4AF97C10.9020208@gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.00.0911100706120.7059@melkinpaasi.cs.helsinki.fi>

Ilpo Järvinen wrote:
> I guess most of the cookie stuff have nothing to do with the next, 
> please make them separate:
> 
>> Redefine two TCP header functions to accept TCP header pointer.
>> When subtracting, return signed int to allow error checking.
> 
> Convert the users here, not in the fifth patch to avoid noise in the large 
> fifth patch.
> 
These comments are directly contrary to advice back on the 2nd or 3rd
round, where I'd both converted and removed the old functions here, before
going on to use them everywhere.  I was told to *only* define the new ones
here, use them in later patches as they applied, and then remove the old
functions in a final cleanup patch.

When I'm done (apparently very far in the distant future), there will be
approximately 100 (maybe 150 or more) uses.


> And, I read more that fifth patch... seriously, please consider now to 
> apply _all_ the coding style changes that have been brought to your 
> attention by multiple people (you should know them now but for some reason 
> again you choose to resent without complying -- I guess on purpose) into 
> all upcoming patches you submit.
> 
Pardon me, but I'm having difficulty finding a substantive comment.  Are
you attempting humor or sarcasm?  If so, it's not readily apparent.  It
seems more of an /ad hominem/ attack to me.

There were many painstaking hours of coding style changes, every single
one of them had to be individually examined side by side for correctness
after I ran a series of regexp over the patches -- in 148 blocks (I just
checked) of diff between the patch diffs, comprising eye-glazing trivial
changes (although I used BBEdit's very nice GUI utility for the
side-by-side comparison).

If you don't like something else, please be specific (as others did
privately).  There may be places the regexp missed.

But more importantly, I'd prefer actual analysis.  Is there a better
Linux function to use for something (like RCU that Eric recommended)?

This is the *easy* patch series, based on code previously reviewed.  The
next series are much more conceptually difficult.  Maybe that's the
problem, this is too easy, and so the only things people find to comment
about are trivia.

Please, more thoughtful code review, less interpersonal sniping.

TIA.


  reply	other threads:[~2009-11-10 14:43 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-09 16:07 [net-next-2.6 PATCH v5 0/5 RFC] TCPCT part1: cookie option exchange William Allen Simpson
2009-11-09 16:12 ` [net-next-2.6 PATCH v5 1/5 RFC] TCPCT part 1a: add request_values parameter for sending SYNACK William Allen Simpson
2009-11-09 16:24 ` [net-next-2.6 PATCH v5 2/5 RFC] TCPCT part1b: TCP_MSS_DEFAULT, TCP_MSS_DESIRED William Allen Simpson
2009-11-09 16:39   ` Eric Dumazet
2009-11-09 16:33 ` [net-next-2.6 PATCH v5 3/5 RFC] TCPCT part1c: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions William Allen Simpson
2009-11-10  5:31   ` Ilpo Järvinen
2009-11-10 14:43     ` William Allen Simpson [this message]
2009-11-09 16:50 ` [net-next-2.6 PATCH v5 4/5 RFC] TCPCT part1d: generate Responder Cookie William Allen Simpson
2009-11-09 17:05 ` [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data William Allen Simpson
2009-11-10  5:05   ` Ilpo Järvinen
2009-11-10 13:41     ` William Allen Simpson
2009-11-10 14:00       ` Ilpo Järvinen
2009-11-10 14:30       ` Ilpo Järvinen
2009-11-10 16:49         ` William Allen Simpson
2009-11-11  1:48           ` Ilpo Järvinen
2009-11-10 14:29   ` Eric Dumazet
2009-11-10 15:45     ` William Allen Simpson
2009-11-11  1:32       ` Ilpo Järvinen
2009-11-11 17:35         ` William Allen Simpson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4AF97C10.9020208@gmail.com \
    --to=william.allen.simpson@gmail.com \
    --cc=ilpo.jarvinen@helsinki.fi \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).