Date:	Tue, 10 Nov 2009 11:49:37 -0500
From:	William Allen Simpson <william.allen.simpson@...il.com>
To:	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
CC:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Joe Perches <joe@...ches.com>
Subject: Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange
 with SYNACK data

Ilpo Järvinen wrote:
> After some time I returned to it. My apologies for my previous harsh 
> response. My point was to say that you must separate such changes because 
> they add to noise which makes it rather annoying to review. At least my 
> brains are rather limited still in keeping track of different things.
> 
Apology accepted.

I'd think we agree, and that's why I wouldn't go around fixing all the
misspelled comments -- just those in the code I'm actually revising.


>> This *is* actually in CodingStyle (line 679), and I'm trying to conform:
> 
> Sure, fell free to but not in some random places like you now do in this 
> patch. If you are not touching a line because of a real change and want do 
> a syntax cleanup, please do it in another patch. Especially when your 
> actual change is as complicated as this is.
> 
Not a single one is in a random place.


>>> ...Also some comment changes which certainly are not mandatory nor even
>>> related.
>>>
>> 1) The first is a hole left by the removal of the data fields some time
>> ago, but they left the old (now incorrect) comment:
>>
>> -/*	SACKs data	*/
>> +	u8	cookie_plus:6,	/* bytes in authenticator/cookie option	*/
>> +		cookie_out_never:1,
>> +		cookie_in_always:1;
>>  	u8	num_sacks;	/* Number of SACK blocks		*/
>>...
> I'm sorry but this response tells me that you don't seem to know even 
> yourself what you were submitting. Does this ring a bell:
> 
> -       if (th->doff == sizeof(struct tcphdr) >> 2) {
> +       /* In the spirit of fast parsing, compare doff directly to shifted
> +        * constant values.  Because equality is used, short doff can be
> +        * ignored here, and checked later.
> +        */
> +       if (th->doff == (sizeof(*th) >> 2)) {
> 
Ah, it's become apparent that you didn't click through to any of the
references, internet-drafts, or mailing list discussion, so you don't
know what this patch series is implementing.

One of the great difficulties in tracking down all several hundred uses
of doff -- and functions that use doff -- is the serious deficiency of
comments (compared to other code bases).

Oh yeah -- Miller says I'm anal.  This is TCP.  Strict scrutiny is much
better than the alternative.


> Besides, I don't understand even what you're trying to say. ...I think we 
> already checked the length in tcp_v[46]_rcv against underflow. Why you add 
> such a non-sense comment here (plus the sizeof change I covered 
> elsewhere)? This is just a noise to annoy reviewer, nothing else.
> 
You merely think?  You don't know?  (I didn't find one on the code walk
through that got me to this point in the code, but perhaps I missed
something.)  If that's the case, there should at least be a comment that
underflow was already checked, and *where* it was checked!

The lack of pertinent comments and error checking annoys the next coder
that comes down the pike.


>> 2) The second is a spelling error that I noticed in passing.  It's within
>> the same diff -u area (close to other insertions both before and after), so
>> fixing it was trivial:
>>
>> - * to increse this, although since:
>> + * to increase this, although since:
>>
>> Are we not supposed to fix spelling errors in comments?
> 
> How many extra lines a reviewer has to read because of that? Why not do it 
> in a separate patch. git add -i / git stash would help you there. ...To be 
> honest, I'd rather have typos in comments which like this are not 
> trouble for the reader than clutter feature patches with all this random 
> junk.
> 
I'll have to look up the usage of git add -i / git stash, but in any case,
I'm not likely to go around fixing comments later.

Each patch requires a great deal of time.  I'd rather combine such things,
or not do them at all.


> ...I really wish that next time when you submit you get rid of all these 
> showstoppers and annoyances (ie., at least honestly try to do your best)
> that we could finally concentrate to the actual change and reviewing
> that :-). My recommendation is that before hitting the send button you 
> review (read through) your own patches, and if you find something to 
> correct instead of submitting, postpone that until the problem is solved. 
> Like DaveM once noted somebody, you'll not be timed :-). ...I usually do 
> that (read my own patches) and end up rather often to not submit (and find 
> even real bugs that way quite often).
> 
Maybe you've been dealing too much with some folks that submitted a patch
that was accepted (over my expressed reservations), and then had to send
out 2 emergency patches to handle crashing bugs last week....

Before I send any patch, it's been compared against my last patch sent,
*and* tested for compilation individually, *and* _usually_ tested pretty
thoroughly -- although not this time, because the net-next-2.6 modules
don't compile, as I warned at the head of this patch series.

Because the compile blew up, I actually spent my Sunday evening
re-reviewing the patches again!  I wanted to ensure it wasn't something
that I'd done, so I recompiled everything again incrementally.

It took me more than a day to make the syntax changes requested, and
another 6 hours to prepare and send these 5 patches.

I cannot promise there will never be any bugs, this patch series will be
much too complicated for that, but the very idea that I'd send without
reviewing....  At this point, I'm rather personally insulted.

And now I'm done with email for the day.  I'll do the split that Eric
suggested, and send it out whenever the damn modules compile again.

In the meantime, I'd appreciate that you "finally concentrate"....
--
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