[B.A.T.M.A.N.] [PATCH] batman-adv: move neigh_node->if_incoming->if_status check in find_router()

[B.A.T.M.A.N.] [PATCH] batman-adv: move neigh_node->if_incoming->if_status check in find_router()

[Antonio Quartulli]

Every time that find_router() is invoked, if_status has to be compared
with IF_ACTIVE. Moving this comparison inside find_router() will avoid to
write it each time.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 routing.c |    4 +++-
 unicast.c |    3 ---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/routing.c b/routing.c
index 49f5715..c875164 100644
--- a/routing.c
+++ b/routing.c
@@ -1237,7 +1237,6 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 
 	/* find the orig_node which has the primary interface. might
 	 * even be the same as our router_orig in many cases */
-
 	if (compare_eth(router_orig->primary_addr, router_orig->orig)) {
 		primary_orig_node = router_orig;
 	} else {
@@ -1266,6 +1265,9 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 		router = find_ifalter_router(primary_orig_node, recv_if);
 
 return_router:
+	if (router && router->if_incoming->if_status != IF_ACTIVE)
+		router = NULL;
+
 	rcu_read_unlock();
 	return router;
 }
diff --git a/unicast.c b/unicast.c
index b46cbf1..1be53d7 100644
--- a/unicast.c
+++ b/unicast.c
@@ -314,9 +314,6 @@ find_router:
 	if (!neigh_node)
 		goto out;
 
-	if (neigh_node->if_incoming->if_status != IF_ACTIVE)
-		goto out;
-
 	if (my_skb_head_push(skb, sizeof(struct unicast_packet)) < 0)
 		goto out;
 
-- 
1.7.3.4

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: move neigh_node->if_incoming->if_status check in find_router()

[Marek Lindner]

On Thursday 05 May 2011 10:21:42 Antonio Quartulli wrote:
>  return_router:
> +       if (router && router->if_incoming->if_status != IF_ACTIVE)
> +               router = NULL;
> +
>         rcu_read_unlock();
>         return router;

You are breaking the reference counting of 'router' here. While looking at 
your patch I found another refcount imbalance. Check the patch I just posted 
(Fix refcount imbalance in find_router).

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: move neigh_node->if_incoming->if_status check in find_router()

[Antonio Quartulli]

On gio, mag 05, 2011 at 02:20:10 +0200, Marek Lindner wrote:
> On Thursday 05 May 2011 10:21:42 Antonio Quartulli wrote:
> >  return_router:
> > +       if (router && router->if_incoming->if_status != IF_ACTIVE)
> > +               router = NULL;
> > +
> >         rcu_read_unlock();
> >         return router;
> 
> You are breaking the reference counting of 'router' here. While looking at 
> your patch I found another refcount imbalance. Check the patch I just posted 
> (Fix refcount imbalance in find_router).

I see :)
And thanks for reviewing.

Regards,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[B.A.T.M.A.N.] [PATCHv2] batman-adv: move neigh_node->if_incoming->if_status check in find_router()

[Antonio Quartulli]

Every time that find_router() is invoked, if_status has to be compared with
IF_ACTIVE. Moving this comparison inside find_router() will avoid to write it
each time.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
In this version the reference counting of 'router' has been corrected.

 routing.c |    3 +++
 unicast.c |    3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/routing.c b/routing.c
index bb1c3ec..8c403ce 100644
--- a/routing.c
+++ b/routing.c
@@ -1240,6 +1240,9 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 		router = find_ifalter_router(primary_orig_node, recv_if);
 
 return_router:
+	if (router && router->if_incoming->if_status != IF_ACTIVE)
+		goto err_unlock;
+
 	rcu_read_unlock();
 	return router;
 err_unlock:
diff --git a/unicast.c b/unicast.c
index 19c3daf..bab6076 100644
--- a/unicast.c
+++ b/unicast.c
@@ -314,9 +314,6 @@ find_router:
 	if (!neigh_node)
 		goto out;
 
-	if (neigh_node->if_incoming->if_status != IF_ACTIVE)
-		goto out;
-
 	if (my_skb_head_push(skb, sizeof(struct unicast_packet)) < 0)
 		goto out;
 
-- 
1.7.3.4

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: move neigh_node->if_incoming->if_status check in find_router()

[Marek Lindner]

On Sunday 08 May 2011 20:52:57 Antonio Quartulli wrote:
> Every time that find_router() is invoked, if_status has to be compared with
> IF_ACTIVE. Moving this comparison inside find_router() will avoid to write
> it each time.

Applied in revision de7607d.

Thanks,
Marek