Date:	Mon, 02 Nov 2009 14:31:10 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	William Allen Simpson <william.allen.simpson@...il.com>
CC:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange
 with SYNACK data

William Allen Simpson a écrit :
> Eric Dumazet wrote:
>> This part is really hard to review, and might be splitted ?
>>
>> cleanups could be done in a cleanup patch only
>>
>> Examples:
>>
>> -    tmp_opt.mss_clamp = 536;
>> -    tmp_opt.user_mss  = tcp_sk(sk)->rx_opt.user_mss;
>> +    tmp_opt.mss_clamp = TCP_MIN_RCVMSS;
>> +    tmp_opt.user_mss  = tp->rx_opt.user_mss;
>>
>>
>> -    tp->mss_cache = 536;
>> +    tp->mss_cache = TCP_MIN_RCVMSS;
>>
> Often hard to decide what's "cleanup" and what's essential.  I'll look at
> that again for the next round, but I've already split the original single
> patch into multiple parts.

cleanups are trivial, and should be separated from functionnal changes.

> 
> 
>> Also your tests are reversed, if you look at the existing coding style.
>>
> I checked Documentation/CodingStyle, and that's not specified.  I've seen
> plenty of examples of modern security coding style around here.
> 
> As a long-time (25+ years) consultant and 30 years C programmer, I'm
> heedful of the project coding style, and had to endure many variants.
> 
> Where I'm working with others' code, you'll note that I keep the same
> style, no matter how ugly, as that makes patches easier to read.
> 
> 
>> Example :
>>
>> +    /* TCP Cookie Transactions */
>> +    if (0 < sysctl_tcp_cookie_size) {
>> +        /* Default, cookies without s_data. */
>> +        tp->cookie_values =
>> +            kzalloc(sizeof(*tp->cookie_values),
>> +                sk->sk_allocation);
>> +        if (NULL != tp->cookie_values)
>> +            kref_init(&tp->cookie_values->kref);
>> +    }
>>
>> should be ->
>>
>> +    /* TCP Cookie Transactions */
>> +    if (sysctl_tcp_cookie_size > 0) {
>> +        /* Default, cookies without s_data. */
>> +        tp->cookie_values =
>> +            kzalloc(sizeof(*tp->cookie_values),
>> +                sk->sk_allocation);
>> +        if (tp->cookie_values != NULL)
>> +            kref_init(&tp->cookie_values->kref);
>> +    }
>>
> And "tp->cookie_values != NULL" is egregiously poor C practice.  It's very
> hard for code review to ensure that didn't get truncated to "= NULL".  The
> important visual element is the NULL, not the variable name.

Maybe, but check in linux source code and you'll see this poor pratice is the facto.
Dont try to change our minds, because it wont happen.

> 
> Also, avoid "!tp->cookie_values", as this is *not* a boolean.

Oh, good to learn that ! operator only applies to boolean. I didnt know that.

> 
> When I'm adding new code, I use constant-to-the-left security coding style,
> as they teach in modern universities (lately also for PHP).  And this is a
> security extension, so a security style is particularly appropriate.
> 
> As in switch statements, constant-to-the-left makes the value obvious,
> especially in a series (and assists transforming if series into a switch).
> 
> For complex tests, this makes the code much more readable and easier to
> visually verify on code walk-through:
> 
> +    if (0 < tmp_opt.cookie_plus
> +     && tmp_opt.saw_tstamp
> +     && !tp->cookie_out_never
> +     && (0 < sysctl_tcp_cookie_size
> +      || (NULL != tp->cookie_values
> +       && 0 < tp->cookie_values->cookie_desired))) {
> 
> Consistent use of security style would have obviated a lot of foolish >= 0
> tests that seem to be constantly in need of fixing.  It's a bad idea to
> depend on the compiler to catch non-executable code.

You can _talk_, I can stop reviewing your patches, and wait another gentle guy do the job,
because I am 30 years experimented (and tired ?) programmer, and dont want to
lose my time to discuss Coding-Style with you ?

Cooking patches to linux is not only matter of good ideas and programming (and Dropping
patches for the masses).

Its also a matter of convincing _people_ that your additions will be maintainable
when you leave kernel programming and let people like us correct bugs.

For the moment, I am not convinced at all. I prefer to talk now.


Note: I did read your TCPCT 25 pages documentation and very am interested by this
improvement, but its _also_ important to implement it in the normal way.
(I wish this document could be public in a RFC form)

--
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