Re: [PATCH] tiny af_packet.c cleanup

2 messages, 2 authors, 2003-09-13 · open the first message on its own page

Re: [PATCH] tiny af_packet.c cleanup

From: Francois Romieu <romieu@fr.zoreil.com>
Date: 2003-09-13 07:35:59

Mitchell Blank Jr [off-list ref] :
[...]
quoted hunk
--- linux-2.6.0-test5-VIRGIN/net/packet/af_packet.c	2003-09-08 12:39:53.000000000 -0700
+++ linux-2.6.0-test5mnb1/net/packet/af_packet.c	2003-09-12 13:28:53.857179768 -0700
@@ -433,9 +433,9 @@
 		struct sk_filter *filter;
 
 		bh_lock_sock(sk);
-		if ((filter = sk->sk_filter) != NULL)
-			res = sk_run_filter(skb, sk->sk_filter->insns,
-					    sk->sk_filter->len);
+		filter = sk->sk_filter;
+		if (likely(filter != NULL))	/* re-check under lock */
+			res = sk_run_filter(skb, filter->insns, filter->len);
1 - pointers tests against NULL are naturally supposed "unlikely" by gcc.
2 - I am not completely convinced that the "/* re-check under lock */" 
    comment is really useful either: the lock statement is on the line before.

Make it more spartan:
--- linux-2.6.0-test5-VIRGIN/net/packet/af_packet.c	2003-09-08 12:39:53.000000000 -0700
+++ linux-2.6.0-test5mnb1/net/packet/af_packet.c	2003-09-12 13:28:53.857179768 -0700
@@ -433,9 +433,9 @@
 		struct sk_filter *filter;
 
 		bh_lock_sock(sk);
-		if ((filter = sk->sk_filter) != NULL)
-			res = sk_run_filter(skb, sk->sk_filter->insns,
-					    sk->sk_filter->len);
+		filter = sk->sk_filter;
+		if (filter)
+			res = sk_run_filter(skb, filter->insns, filter->len);
 		bh_unlock_sock(sk);
 
 		if (res == 0)
@@ -537,9 +537,9 @@
 		struct sk_filter *filter;
 
 		bh_lock_sock(sk);
-		if ((filter = sk->sk_filter) != NULL)
-			res = sk_run_filter(skb, sk->sk_filter->insns,
-					    sk->sk_filter->len);
+		filter = sk->sk_filter;
+		if (filter)
+			res = sk_run_filter(skb, filter->insns, filter->len);
 		bh_unlock_sock(sk);
 
 		if (res == 0)

Re: [PATCH] tiny af_packet.c cleanup

From: Mitchell Blank Jr <mitch@sfgoth.com>
Date: 2003-09-13 08:02:52

Francois Romieu wrote:
2 - I am not completely convinced that the "/* re-check under lock */" 
    comment is really useful either: the lock statement is on the line before.
I thought that without the comment someone might think that the second
"if()" wasn't needed (since we had just checked the same value against
NULL a few lines up)

-Mitch
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help