netdev - Re: [PATCH net-next 2/3] tipc: message reassembly using fragment chain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 29 Oct 2013 09:56:56 +0800
From:	Ying Xue <ying.xue@...driver.com>
To:	Jon Maloy <maloy@...jonn.com>
CC:	<jon.maloy@...csson.com>, <paul.gortmaker@...driver.com>,
	<erik.hugne@...csson.com>, <tipc-discussion@...ts.sourceforge.net>,
	<netdev@...r.kernel.org>, David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next 2/3] tipc: message reassembly using fragment
 chain

On 10/28/2013 06:24 PM, Jon Maloy wrote:
> On 10/28/2013 01:07 AM, David Miller wrote:
>> From: Jon Maloy <jon.maloy@...csson.com>
>> Date: Sat, 26 Oct 2013 14:41:02 -0400
>>
>>> +			int ret = tipc_link_recv_fragment(
>>> +					&node->bclink.reasm_head,
>>> +					&node->bclink.reasm_tail,
>>> +					&buf);
>> This is not the correct way to indent a function call that spans
>> multiple lines.  In such a situation the arguments that appear
>> on the second and subsequent lines must start at the first column
>> after the openning parenthesis of the function call.
>>
>> Like this:
>>
>> 	func(a, b, c,
>> 	     d, e, f);
>>
>> Please audit this in your entire set of patches and resubmit,
>> thanks.
> 
> Doing as David says here means that some lines will be >80 chars.
> This was the reason for the somewhat strange indentation.
> I tried to rename the function to tipc_link_rcv_fragm(), but one
> line was still too long. The problem we have goes deeper.
> 
> In Linus' coding style manual I read that the 80 char limit is a hard limit,
> a limit we violate in several places. One offender is that we have too
> many indentation levels, at least in tipc_recv_msg() and probably in
> some other places. This is sensitive code, that I don't feel for touching
> right now.  A more low hanging fruit is our local variable names:
> names such as l_ptr, n_ptr, b_ptr is exactly what Linus characterizes
> as "brain dead Hungarian style", and I never liked that naming anyway.
> For me l, n, and b is good enough as long as the context is clear.
> 
> But, doing so, at least in tipc_recv_msg(), would require another, separate,
> patch, and it would lead to style inconsistency.
> 
> In brief, I am at loss about to proceed here, and I am not going to submit
> this patch again until I have some feedback from somebody who can tell me
> what is the right thing to do. Maybe > 80 chars is fine for now?
> 

It's hard completely resolve the issue by simply changing variable
names. So in my opinion, this is not a simple code style problem any
more. As tipc_recv_msg() is too complex, it includes at least three
embedded loops so that it doesn't remain too much space for functions in
the most inner loop. This is the key point.

Therefore, the best method is to divide tipc_recv_msg() into several
smaller functions to simplify the current implementation. But it's not
an easy job. Actually I ever tried to do it, but I gave up lastly
because I did not find one perfect solution about how to divide it.

Regards,
Ying

> ///jon
> 
> 
> 
> 

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