finding a connected ROSE neighbor node

Hi,

This patch introduces a test to determine if a neighbor node with a given ROSE address is already connected and if so returns its address. The previous test was only performed on timer t0 and not on parameter restarted that is true when a node is connected. Also, previously the test was not performed on the whole node list.
A new function __rose_get_neigh() is introduced to cure a spin lock conflict related to rose_node_list_lock.

signed off by Alexey Dobriyan,[email protected]
signed off by Bernard Pidoux, f6bvp @ amsat.org

@@ -202,6 +202,7 @@
 extern struct net_device *rose_dev_get(rose_address *);
 extern struct rose_route *rose_route_free_lci(unsigned int, struct rose_neigh *);
 extern struct rose_neigh *rose_get_neigh(rose_address *, unsigned char *, unsigned char *);
+extern struct rose_neigh *__rose_get_neigh(rose_address *, unsigned char *, unsigned char *);
 extern int  rose_rt_ioctl(unsigned int, void __user *);
 extern void rose_link_failed(ax25_cb *, int);
 extern int  rose_route_frame(struct sk_buff *, ax25_cb *);

@@ -664,25 +664,22 @@
 /*
  *	Find a neighbour given a ROSE address.
  */
-struct rose_neigh *rose_get_neigh(rose_address *addr, unsigned char *cause,
+struct rose_neigh *__rose_get_neigh(rose_address *addr, unsigned char *cause,
 	unsigned char *diagnostic)
 {
-	struct rose_neigh *res = NULL;
 	struct rose_node *node;
 	int failed = 0;
 	int i;
 
-	spin_lock_bh(&rose_node_list_lock);
 	for (node = rose_node_list; node != NULL; node = node->next) {
 		if (rosecmpm(addr, &node->address, node->mask) == 0) {
 			for (i = 0; i < node->count; i++) {
-				if (!rose_ftimer_running(node->neighbour[i])) {
-					res = node->neighbour[i];
-					goto out;
-				} else
-					failed = 1;
+				if (node->neighbour[i]->restarted)
+					return node->neighbour[i];
+				if (!rose_ftimer_running(node->neighbour[i]))			
+					return node->neighbour[i];
+				failed = 1;
 			}
-			break;
 		}
 	}
 

@@ -694,7 +691,16 @@
 		*diagnostic = 0;
 	}
 
-out:
+	return NULL;
+}
+
+struct rose_neigh *rose_get_neigh(rose_address *addr, unsigned char *cause,
+	unsigned char *diagnostic)
+{
+	struct rose_neigh *res;
+
+	spin_lock_bh(&rose_node_list_lock);
+	res = __rose_get_neigh(addr, cause, diagnostic);
 	spin_unlock_bh(&rose_node_list_lock);
 
 	return res;

@@ -1019,7 +1025,7 @@
 		rose_route = rose_route->next;
 	}
 
-	if ((new_neigh = rose_get_neigh(dest_addr, &cause, &diagnostic)) == NULL) {
+	if ((new_neigh = __rose_get_neigh(dest_addr, &cause, &diagnostic)) == NULL) {
 		rose_transmit_clear_request(rose_neigh, lci, cause, diagnostic);
 		goto out;
 	}

@@ -750,7 +750,7 @@
 	sk->sk_state   = TCP_CLOSE;
 	sock->state = SS_UNCONNECTED;
 
-	rose->neighbour = rose_get_neigh(&addr->srose_addr, &cause,
+	rose->neighbour = __rose_get_neigh(&addr->srose_addr, &cause,
 					 &diagnostic);
 	if (!rose->neighbour)
 		return -ENETUNREACH;

Hi,

I propose to drop the previously submitted patch for I performed more investigations on ROSE frame routing and found that it was not completely satisfactory.

Please find here another commit to net-2.6.25 with explanations.

From fd66cc115e058b2fc63a0e26aa73f1d27113105a Mon Sep 17 00:00:00 2001
From: Bernard Pidoux <[email protected]>
Date: Thu, 10 Jan 2008 23:10:44 +0100
Subject: [PATCH 1/4] [ROSE] new rose_get_route() function

rose_get_neigh() was called by two different functions. Firstly, by rose_connect() in order to establish connections to adjacent rose nodes. This worked correctly.
Secondly, it was called by rose_route_frame() to find a route via an adjacent node. This was not working efficiently, for the proper test was not performed in order to check if the node was already connected.
A new function rose_get_route() is devoted to frame routing. It returns a ROSE node address for sending a frame to the specified destination via a connected node.

---
 net/rose/rose_route.c |   34 +++++++++++++++++++++++++++++++++-
 1 files changed, 33 insertions(+), 1 deletions(-)

index 540c0f2..ec79567 100644
@@ -662,6 +662,38 @@ struct rose_route *rose_route_free_lci(unsigned int lci, struct rose_neigh *neig
 }
 
 /*
+ *     Find an opened route given a ROSE address.
+ */
+struct rose_neigh *rose_get_route(rose_address *addr, unsigned char *cause,
+       unsigned char *diagnostic)
+{
+       struct rose_node *node;
+       int failed = 0;
+       int i;
+
+       for (node = rose_node_list; node != NULL; node = node->next) {
+               if (rosecmpm(addr, &node->address, node->mask) == 0) {
+                       for (i = 0; i < node->count; i++) {
+                               if (node->neighbour[i]->restarted)
+                                       return node->neighbour[i];
+                               failed = 1;
+                       }
+               }
+       }
+
+       if (failed) {
+               *cause      = ROSE_OUT_OF_ORDER;
+               *diagnostic = 0;
+       } else {
+               *cause      = ROSE_NOT_OBTAINABLE;
+               *diagnostic = 0;
+       }
+
+       return NULL;
+}
+
+
+/*
  *     Find a neighbour given a ROSE address.
  */
 struct rose_neigh *rose_get_neigh(rose_address *addr, unsigned char 

*cause,
@@ -1019,7 +1051,7 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25)

                rose_route = rose_route->next;
        }

- if ((new_neigh = rose_get_neigh(dest_addr, &cause, &diagnostic)) == NULL) {
+ if ((new_neigh = rose_get_route(dest_addr, &cause, &diagnostic)) == NULL) {
rose_transmit_clear_request(rose_neigh, lci, cause,

diagnostic);
                goto out;
        }

--
1.5.3.7

Hi,

This patch is send to replace preceding commit :

From fd66cc115e058b2fc63a0e26aa73f1d27113105a Mon Sep 17 00:00:00 2001
From: Bernard Pidoux <[email protected]>
Date: Thu, 10 Jan 2008 23:10:44 +0100
Subject: [PATCH 1/4] [ROSE] new rose_get_route() function

I removed unnecessary lines of code copied from rose_get_neigh().
The function is now called with fewer arguments and if
a NULL is returned then the next call to rose_transmit_clear_request()
will manage the error code. I verified that the frame routing was
actually handled as required on a FPAC/ROSE node with heavy BBS traffic through 8 adjacent nodes connected through radio ports and Internet AXUDP / AXIP connections.

From 6708b3853a5ecf5c185942b3757223e451af1960 Mon Sep 17 00:00:00 2001
From: Bernard Pidoux <[email protected]>
Date: Sun, 13 Jan 2008 20:25:40 +0100
Subject: [PATCH 1/4] [ROSE] simplified rose_get_route()

rose_get_neigh() was called by two different functions.
Firstly, by rose_connect() in order to establish connections to
adjacent rose nodes. This worked correctly.
Secondly, it was called by rose_route_frame() to find a route
via an adjacent node. This was not working efficiently, for the
whole node neighbour list was not checked and the proper test
was not performed in order to check if a node was already connected.
We create new function rose_get_route() to achieve this.
It returns a ROSE node address for sending a frame via a connected node
to the specified destination address.If a NULL is returned, the
previous program behavior is performed. ROSE tries to initiate a connect to the appropriate adjacent node.

---
 net/rose/rose_route.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

index 540c0f2..5d6fac4 100644
@@ -662,6 +662,25 @@ struct rose_route *rose_route_free_lci(unsigned int lci, struct rose_neigh *neig
 }
 
 /*
+ *     Find an opened route given a ROSE address.
+ */
+static struct rose_neigh *rose_get_route(rose_address *addr)
+{
+       struct rose_node *node;
+       int i;
+
+       for (node = rose_node_list; node != NULL; node = node->next) {
+               if (rosecmpm(addr, &node->address, node->mask) == 0) {
+                       for (i = 0; i < node->count; i++) {
+                               if (node->neighbour[i]->restarted)
+                                       return node->neighbour[i];
+                       }
+               }
+       }
+       return NULL;
+}
+
+/*
  *     Find a neighbour given a ROSE address.
  */
 struct rose_neigh *rose_get_neigh(rose_address *addr, unsigned char 

*cause,
@@ -842,7 +861,6 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25)

        struct sock *sk;
        unsigned short frametype;
        unsigned int lci, new_lci;
-       unsigned char cause, diagnostic;
        struct net_device *dev;
        int len, res = 0;
        char buf[11];
@@ -1019,8 +1037,8 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25)
                rose_route = rose_route->next;
        }

- if ((new_neigh = rose_get_neigh(dest_addr, &cause, &diagnostic)) == NULL) {
- rose_transmit_clear_request(rose_neigh, lci, cause, diagnostic);
+ if ((new_neigh = rose_get_route(dest_addr)) == NULL) {

+               rose_transmit_clear_request(rose_neigh, lci, ROSE_NOT_OBTAINABLE, 0);
                goto out;
        }

--
1.5.3.7

From 46bccce1e660a39bcc8f8cf87d4c17de33f4ba48 Mon Sep 17 00:00:00 2001
From: Bernard Pidoux <[email protected]>
Date: Thu, 10 Jan 2008 23:01:46 +0100
Subject: [PATCH 2/4] [ROSE] template declaration for rose_get_route()

---
 include/net/rose.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index e5bb084..d3ab453 100644
@@ -202,6 +202,7 @@ extern struct net_device *rose_dev_first(void);
 extern struct net_device *rose_dev_get(rose_address *);
 extern struct rose_route *rose_route_free_lci(unsigned int, struct 

rose_neigh *);
extern struct rose_neigh *rose_get_neigh(rose_address *, unsigned char
*, unsigned char *);
+extern struct rose_neigh *rose_get_route(rose_address *, unsigned char *, unsigned char *);
extern int rose_rt_ioctl(unsigned int, void __user *);
extern void rose_link_failed(ax25_cb *, int);
extern int rose_route_frame(struct sk_buff *, ax25_cb *);
--
1.5.3.7

Strange... if rose_get_route() is used only in net/rose/rose_route.c, why dont you define it static, and not extern in include/net/rose.h ?

I agree. You are perfectly right.
There is no need to declare rose_get_route() external.
I stupidly copied rose_get_neigh()definition from which I derived rose_get_route();

Also I am not sure that setting cause and diagnostic is necessary, as they are not used by the calling function.

By the way. I made a typo in [PATCH 4/4].

Instead of "Initial connection to rose neighbour nodes was unusually, t0 timer was blocked and application program could not
use socket."

One should read
"Initial connection to rose neighbour nodes was unusually, long, t0 timer was blocked and application program could not use socket."

Bernard P.

Hi,

I wrote a "simplified" get_route() function.
It is declared as static following judicious Eric's remark.

Then, the following patch of include/net/rose.h is no more necessary.

A new commit for rose_get_route() will be presented in next message.

Thank you Eric for pushing me to reexamine my code.

Regards,

Bernard P.

From bc108e5ee0b0353c3707df25e12e40038da0160a Mon Sep 17 00:00:00 2001
From: Bernard Pidoux <[email protected]>
Date: Fri, 11 Jan 2008 10:23:55 +0100
Subject: [PATCH 3/4] [ROSE] return with lock held

================================================
[ BUG: lock held when returning to user space! ]

------------------------------------------------
fpacwpd/3057 is leaving the kernel with locks still held!
1 lock held by fpacwpd/3057:
#0: (sk_lock-AF_ROSE){--..}, at: [<d8bfda7f>] rose_connect+0x6c/0x357 [rose]

---
 net/rose/af_rose.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

index 8f70ad8..9419946 100644
@@ -752,7 +752,8 @@ static int rose_connect(struct socket *sock, struct sockaddr *uaddr, int addr_le
        rose->neighbour = rose_get_neigh(&addr->srose_addr, &cause,
                                         &diagnostic);
        if (!rose->neighbour)
-               return -ENETUNREACH;
+               err = -ENETUNREACH;
+               goto out_release;
 
        rose->lci = rose_new_lci(rose->neighbour);
        if (!rose->lci) {

--
1.5.3.7

From 5c50971c6088f380eafdb1a6a7de5a5d3686c8c7 Mon Sep 17 00:00:00 2001
From: Bernard Pidoux <[email protected]>
Date: Sat, 12 Jan 2008 20:19:34 +0100
Subject: [PATCH 4/4] [ROSE] ENETUNREACH held rose_connect()

Initial connection to rose neighbour nodes was unusually, t0 timer was blocked and application program could not use socket.
I performed a number of trials in order to find what was slowing rose_connect() and blocking ROSE t0 timer. Replacing the type of error returned did not cure the problem. Finally removing the error returned when rose->neighbour is NULL is the solution.
Without err = -ENETUNREACH rose_connect() returns default err = 0. Doing that, t0 timer is running, rose_connect() performs adjacent rose node connections as needed and rose frames are well routed. Also, application programs can access rose connect socket.

---
 net/rose/af_rose.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

index 9419946..5a8c886 100644
@@ -752,7 +752,6 @@ static int rose_connect(struct socket *sock, struct sockaddr *uaddr, int addr_le
        rose->neighbour = rose_get_neigh(&addr->srose_addr, &cause,
                                         &diagnostic);
        if (!rose->neighbour)
-               err = -ENETUNREACH;
                goto out_release;
 
        rose->lci = rose_new_lci(rose->neighbour);

--
1.5.3.7