[PATCH] ipv4: remove parentheses in return statement

[PATCH] ipv4: remove parentheses in return statement

[Jean Sacren]

It is always wrong to write 'return (...)'.

After removal, realign the code where necessary.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 net/ipv4/devinet.c    |    4 ++--
 net/ipv4/inetpeer.c   |    2 +-
 net/ipv4/syncookies.c |    8 ++++----
 net/ipv4/tcp_input.c  |   16 ++++++++--------
 net/ipv4/tcp_output.c |    2 +-
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 44bf82e..72b16e4 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -106,8 +106,8 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr)
 {
 	u32 val = (__force u32) addr ^ hash_ptr(net, 8);
 
-	return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
-		(IN4_ADDR_HSIZE - 1));
+	return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
+	       (IN4_ADDR_HSIZE - 1);
 }
 
 static void inet_hash_insert(struct net *net, struct in_ifaddr *ifa)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index e1e0a4e..8049ce0 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -96,7 +96,7 @@ static atomic_t v6_seq = ATOMIC_INIT(0);
 
 static atomic_t *inetpeer_seq_ptr(int family)
 {
-	return (family == AF_INET ? &v4_seq : &v6_seq);
+	return (family == AF_INET) ? &v4_seq : &v6_seq;
 }
 
 static inline void flush_check(struct inet_peer_base *base, int family)
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 650e152..cb9c489 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -103,10 +103,10 @@ static __u32 secure_tcp_syn_cookie(__be32 saddr, __be32 daddr, __be16 sport,
 	 * MSS into the second hash value.
 	 */
 
-	return (cookie_hash(saddr, daddr, sport, dport, 0, 0) +
-		sseq + (count << COOKIEBITS) +
-		((cookie_hash(saddr, daddr, sport, dport, count, 1) + data)
-		 & COOKIEMASK));
+	return cookie_hash(saddr, daddr, sport, dport, 0, 0) +
+	       sseq + (count << COOKIEBITS) +
+	       ((cookie_hash(saddr, daddr, sport, dport, count, 1) + data)
+		& COOKIEMASK);
 }
 
 /*
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2fd2bc9..1d220f9 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3993,17 +3993,17 @@ static int tcp_disordered_ack(const struct sock *sk, const struct sk_buff *skb)
 	u32 seq = TCP_SKB_CB(skb)->seq;
 	u32 ack = TCP_SKB_CB(skb)->ack_seq;
 
-	return (/* 1. Pure ACK with correct sequence number. */
-		(th->ack && seq == TCP_SKB_CB(skb)->end_seq && seq == tp->rcv_nxt) &&
+	return /* 1. Pure ACK with correct sequence number. */
+	       (th->ack && seq == TCP_SKB_CB(skb)->end_seq && seq == tp->rcv_nxt) &&
 
-		/* 2. ... and duplicate ACK. */
-		ack == tp->snd_una &&
+	       /* 2. ... and duplicate ACK. */
+	       ack == tp->snd_una &&
 
-		/* 3. ... and does not update window. */
-		!tcp_may_update_window(tp, ack, seq, ntohs(th->window) << tp->rx_opt.snd_wscale) &&
+	       /* 3. ... and does not update window. */
+	       !tcp_may_update_window(tp, ack, seq, ntohs(th->window) << tp->rx_opt.snd_wscale) &&
 
-		/* 4. ... and sits in replay window. */
-		(s32)(tp->rx_opt.ts_recent - tp->rx_opt.rcv_tsval) <= (inet_csk(sk)->icsk_rto * 1024) / HZ);
+	       /* 4. ... and sits in replay window. */
+	       (s32)(tp->rx_opt.ts_recent - tp->rx_opt.rcv_tsval) <= (inet_csk(sk)->icsk_rto * 1024) / HZ;
 }
 
 static inline bool tcp_paws_discard(const struct sock *sk,
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index a7b3ec9..eac214c 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1557,7 +1557,7 @@ static inline unsigned int tcp_cwnd_test(const struct tcp_sock *tp,
 	in_flight = tcp_packets_in_flight(tp);
 	cwnd = tp->snd_cwnd;
 	if (in_flight < cwnd)
-		return (cwnd - in_flight);
+		return cwnd - in_flight;
 
 	return 0;
 }

Re: [PATCH] ipv4: remove parentheses in return statement

[David Miller]

From: Jean Sacren <sakiwit@gmail.com>
Date: Fri,  3 Aug 2012 01:43:10 -0600

> It is always wrong to write 'return (...)'.

In your imagination.

> After removal, realign the code where necessary.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>

> -	return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> -		(IN4_ADDR_HSIZE - 1));
> +	return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> +	       (IN4_ADDR_HSIZE - 1);

Those parenthesis are there to make the evaluation order and
grouping explicit.

The other ones you changed are wrong for similar reasons.

I absolutely am not applying patches like this.

Re: [PATCH] ipv4: remove parentheses in return statement

[Eric Dumazet]

On Fri, 2012-08-03 at 01:43 -0600, Jean Sacren wrote:

> @@ -106,8 +106,8 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr)
>  {
>  	u32 val = (__force u32) addr ^ hash_ptr(net, 8);
>  
> -	return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> -		(IN4_ADDR_HSIZE - 1));
> +	return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> +	       (IN4_ADDR_HSIZE - 1);
>  }

BTW This should use a faster implementation, I'll send a patch when
net-next is opened.

Re: [PATCH] ipv4: remove parentheses in return statement

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 03 Aug 2012 11:19:44 +0200

> On Fri, 2012-08-03 at 01:43 -0600, Jean Sacren wrote:
> 
>> @@ -106,8 +106,8 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr)
>>  {
>>  	u32 val = (__force u32) addr ^ hash_ptr(net, 8);
>>  
>> -	return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
>> -		(IN4_ADDR_HSIZE - 1));
>> +	return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
>> +	       (IN4_ADDR_HSIZE - 1);
>>  }
> 
> BTW This should use a faster implementation, I'll send a patch when
> net-next is opened.

There seems to be a few spots where we want the pointer "as a 32-bit
integer" for hashing.  We were discussing arp_hashfn() and ndisc_hashfn()
the other day.

It should basically do something like:

	(u32) ((u64)ptr >> 32 | ((u32) ptr))

on 64-bit and simply (u32)(ptr) on 32-bit.

Re: [PATCH] ipv4: remove parentheses in return statement

[Eric Dumazet]

On Fri, 2012-08-03 at 02:22 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 03 Aug 2012 11:19:44 +0200
> 
> > On Fri, 2012-08-03 at 01:43 -0600, Jean Sacren wrote:
> > 
> >> @@ -106,8 +106,8 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr)
> >>  {
> >>  	u32 val = (__force u32) addr ^ hash_ptr(net, 8);
> >>  
> >> -	return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> >> -		(IN4_ADDR_HSIZE - 1));
> >> +	return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> >> +	       (IN4_ADDR_HSIZE - 1);
> >>  }
> > 
> > BTW This should use a faster implementation, I'll send a patch when
> > net-next is opened.
> 
> There seems to be a few spots where we want the pointer "as a 32-bit
> integer" for hashing.  We were discussing arp_hashfn() and ndisc_hashfn()
> the other day.
> 
> It should basically do something like:
> 
> 	(u32) ((u64)ptr >> 32 | ((u32) ptr))
> 
> on 64-bit and simply (u32)(ptr) on 32-bit.

We already have such thing in fact : net_hash_mix() which returns 0 if
NS are not configured.

(hash_ptr(net,8) is really overkill on 64bit arches)

High order bits on "struct net *" have absolutely no entropy, unless you
have a monster machine (more than 256 GB of ram)

This is the patch I prepared :


diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 44bf82e..b9753ab 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -94,25 +94,22 @@ static const struct nla_policy ifa_ipv4_policy[IFA_MAX+1] = {
 	[IFA_LABEL]     	= { .type = NLA_STRING, .len = IFNAMSIZ - 1 },
 };
 
-/* inet_addr_hash's shifting is dependent upon this IN4_ADDR_HSIZE
- * value.  So if you change this define, make appropriate changes to
- * inet_addr_hash as well.
- */
-#define IN4_ADDR_HSIZE	256
+#define IN4_ADDR_HSIZE_SHIFT	8
+#define IN4_ADDR_HSIZE		(1U << IN4_ADDR_HSIZE_SHIFT)
+
 static struct hlist_head inet_addr_lst[IN4_ADDR_HSIZE];
 static DEFINE_SPINLOCK(inet_addr_hash_lock);
 
-static inline unsigned int inet_addr_hash(struct net *net, __be32 addr)
+static u32 inet_addr_hash(struct net *net, __be32 addr)
 {
-	u32 val = (__force u32) addr ^ hash_ptr(net, 8);
+	u32 val = (__force u32) addr ^ net_hash_mix(net);
 
-	return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
-		(IN4_ADDR_HSIZE - 1));
+	return hash_32(val, IN4_ADDR_HSIZE_SHIFT);
 }
 
 static void inet_hash_insert(struct net *net, struct in_ifaddr *ifa)
 {
-	unsigned int hash = inet_addr_hash(net, ifa->ifa_local);
+	u32 hash = inet_addr_hash(net, ifa->ifa_local);
 
 	spin_lock(&inet_addr_hash_lock);
 	hlist_add_head_rcu(&ifa->hash, &inet_addr_lst[hash]);
@@ -136,18 +133,18 @@ static void inet_hash_remove(struct in_ifaddr *ifa)
  */
 struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
 {
-	unsigned int hash = inet_addr_hash(net, addr);
+	u32 hash = inet_addr_hash(net, addr);
 	struct net_device *result = NULL;
 	struct in_ifaddr *ifa;
 	struct hlist_node *node;
 
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(ifa, node, &inet_addr_lst[hash], hash) {
-		struct net_device *dev = ifa->ifa_dev->dev;
-
-		if (!net_eq(dev_net(dev), net))
-			continue;
 		if (ifa->ifa_local == addr) {
+			struct net_device *dev = ifa->ifa_dev->dev;
+
+			if (!net_eq(dev_net(dev), net)
+				continue;
 			result = dev;
 			break;
 		}

Re: [PATCH] ipv4: remove parentheses in return statement

[Jean Sacren]

From: David Miller <davem@davemloft.net>
Date: Fri, 03 Aug 2012 01:52:43 -0700
>
> From: Jean Sacren <sakiwit@gmail.com>
> Date: Fri,  3 Aug 2012 01:43:10 -0600
> 
> > Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> 
> > -	return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> > -		(IN4_ADDR_HSIZE - 1));
> > +	return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) &
> > +	       (IN4_ADDR_HSIZE - 1);
> 
> Those parenthesis are there to make the evaluation order and
> grouping explicit.
> 
> The other ones you changed are wrong for similar reasons.

Barring the speed issue raised by Eric Dumazet, this patch is correct.

To illustrate, the patch merely changes

	return (A);

to

	return A;

where A is nothing but an expression, regardless if it is a simple one
or a compound one.

Whatever A is, it evaluates to a value. The parentheses do _not_
contribute to the evaluation. They do not alter precedence. They do not
make the evaluation order explicit. They are there to serve no purpose,
yet they do make return statement look like a function call.

Moreover, A is one single entity. It does not group with anything else.
I really don't see the significance of making A a group by itself.

Thank you for reviewing the patch. With all due respect, your assessment
is hard to agree with this moment.

-- 
Jean Sacren

Re: [PATCH] ipv4: remove parentheses in return statement

[David Miller]

From: Jean Sacren <sakiwit@gmail.com>
Date: Fri, 3 Aug 2012 14:23:50 -0600

> To illustrate, the patch merely changes
> 
> 	return (A);
> 
> to
> 
> 	return A;

Life is not black and white my friend, there are shades
of gray.  I feel bad for you if you read coding style rules
in a %100 literal sense with no room for interpretation.

People add extra parenthesis to add clarity, so you are
entirely misrepresenting these cases.

This was not a simple case of:

	return (A);

but more like something that looks as:

	return ((A & B) | (C ^ D));

and that is perfectly fine.

Re: [PATCH] ipv4: remove parentheses in return statement

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 03 Aug 2012 11:53:58 +0200

> We already have such thing in fact : net_hash_mix() which returns 0 if
> NS are not configured.
> 
> (hash_ptr(net,8) is really overkill on 64bit arches)
> 
> High order bits on "struct net *" have absolutely no entropy, unless you
> have a monster machine (more than 256 GB of ram)
> 
> This is the patch I prepared :

Looks good to me.