[PATCH v2 2/2] net: netrom: refactor code in nr_add_node

[PATCH v2 2/2] net: netrom: refactor code in nr_add_node

[Gustavo A. R. Silva]

Code refactoring in order to make it easier to read and maintain.

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
This code was tested by compilation only.

Changes in v2:
 Make use of the swap macro and remove inline keyword.

 net/netrom/nr_route.c | 59 ++++++++++++++-------------------------------------
 1 file changed, 16 insertions(+), 43 deletions(-)

diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index fc9cadc..505e142 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -80,6 +80,19 @@ static struct nr_neigh *nr_neigh_get_dev(ax25_address *callsign,
 
 static void nr_remove_neigh(struct nr_neigh *);
 
+/*      re-sort the routes in quality order.    */
+static void re_sort_routes(struct nr_node *nr_node, int x, int y)
+{
+	if (nr_node->routes[y].quality > nr_node->routes[x].quality) {
+		if (nr_node->which == x)
+			nr_node->which = y;
+		else if (nr_node->which == y)
+			nr_node->which = x;
+
+		swap(nr_node->routes[x], nr_node->routes[y]);
+	}
+}
+
 /*
  *	Add a new route to a node, and in the process add the node and the
  *	neighbour if it is new.
@@ -90,7 +103,6 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 {
 	struct nr_node  *nr_node;
 	struct nr_neigh *nr_neigh;
-	struct nr_route nr_route;
 	int i, found;
 	struct net_device *odev;
 
@@ -251,50 +263,11 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 	/* Now re-sort the routes in quality order */
 	switch (nr_node->count) {
 	case 3:
-		if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
-			switch (nr_node->which) {
-			case 0:
-				nr_node->which = 1;
-				break;
-			case 1:
-				nr_node->which = 0;
-				break;
-			}
-			nr_route           = nr_node->routes[0];
-			nr_node->routes[0] = nr_node->routes[1];
-			nr_node->routes[1] = nr_route;
-		}
-		if (nr_node->routes[2].quality > nr_node->routes[1].quality) {
-			switch (nr_node->which) {
-			case 1:  nr_node->which = 2;
-				break;
-
-			case 2:  nr_node->which = 1;
-				break;
-
-			default:
-				break;
-			}
-			nr_route           = nr_node->routes[1];
-			nr_node->routes[1] = nr_node->routes[2];
-			nr_node->routes[2] = nr_route;
-		}
+		re_sort_routes(nr_node, 0, 1);
+		re_sort_routes(nr_node, 1, 2);
 		/* fall through */
 	case 2:
-		if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
-			switch (nr_node->which) {
-			case 0:  nr_node->which = 1;
-				break;
-
-			case 1:  nr_node->which = 0;
-				break;
-
-			default: break;
-			}
-			nr_route           = nr_node->routes[0];
-			nr_node->routes[0] = nr_node->routes[1];
-			nr_node->routes[1] = nr_route;
-			}
+		re_sort_routes(nr_node, 0, 1);
 	case 1:
 		break;
 	}
-- 
2.7.4

Re: [PATCH v2 2/2] net: netrom: refactor code in nr_add_node

[David Miller]

From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date: Sun, 22 Oct 2017 20:08:40 -0500

> Code refactoring in order to make it easier to read and maintain.
> 
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>

Gustavo, always when reposting a new version of a patch that is part of
a series you must _always_ repost the entire patch series.

Also, a proper patch series must begine with a "[PATCH 0/2] ..."
header posting explaining at a high level what the patch series
is doing, how it is doing it, and why it is doing it that way.

Thank you.

Re: [PATCH v2 2/2] net: netrom: refactor code in nr_add_node

[Gustavo A. R. Silva]

Quoting David Miller <davem@davemloft.net>:

> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Sun, 22 Oct 2017 20:08:40 -0500
>
>> Code refactoring in order to make it easier to read and maintain.
>>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> Gustavo, always when reposting a new version of a patch that is part of
> a series you must _always_ repost the entire patch series.
>

OK. I got it.

> Also, a proper patch series must begine with a "[PATCH 0/2] ..."
> header posting explaining at a high level what the patch series
> is doing, how it is doing it, and why it is doing it that way.
>

Yeah, in this case I thought there was no need for this as both  
patches are not actually related in terms of functionality. But now  
that I'm writing this, maybe that is precisely the reason why I should  
have posted such header...?

Thanks
--
Gustavo A. R. Silva

[PATCH v3 0/2] refactor code and mark expected switch fall-throughs

[Gustavo A. R. Silva]

The aim of this patchset is firstly to refactor code in nr_route.c in order to make it
easier to read and maintain and, secondly, to mark some expected switch fall-throughs
in preparation to enabling -Wimplicit-fallthrough.

I have to mention that I did not implement any unit test.
If someone has any suggestions on how I could test this piece of code
it'd be greatly appreciated.

Thanks

Changes in v2:
 - Make use of the swap macro and remove inline keyword as suggested by
   Walter Harms and Kevin Dawson.

Changes in v3:
 - Update subject for both patches.
 - Add this cover letter as suggested by David Miller.

Gustavo A. R. Silva (2):
  net: netrom: nr_route: refactor code in nr_add_node
  net: netrom: nr_route: mark expected switch fall-throughs

 net/netrom/nr_route.c | 62 ++++++++++++++++-----------------------------------
 1 file changed, 19 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] net: netrom: nr_route: refactor code in nr_add_node

[Gustavo A. R. Silva]

Code refactoring in order to make the code easier to read and maintain.

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
 Make use of the swap macro and remove inline keyword.

Changes in v3:
 Update subject.

 net/netrom/nr_route.c | 59 ++++++++++++++-------------------------------------
 1 file changed, 16 insertions(+), 43 deletions(-)

diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index 0c59354..fba4b4c 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -80,6 +80,19 @@ static struct nr_neigh *nr_neigh_get_dev(ax25_address *callsign,
 
 static void nr_remove_neigh(struct nr_neigh *);
 
+/*      re-sort the routes in quality order.    */
+static void re_sort_routes(struct nr_node *nr_node, int x, int y)
+{
+	if (nr_node->routes[y].quality > nr_node->routes[x].quality) {
+		if (nr_node->which == x)
+			nr_node->which = y;
+		else if (nr_node->which == y)
+			nr_node->which = x;
+
+		swap(nr_node->routes[x], nr_node->routes[y]);
+	}
+}
+
 /*
  *	Add a new route to a node, and in the process add the node and the
  *	neighbour if it is new.
@@ -90,7 +103,6 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 {
 	struct nr_node  *nr_node;
 	struct nr_neigh *nr_neigh;
-	struct nr_route nr_route;
 	int i, found;
 	struct net_device *odev;
 
@@ -251,49 +263,10 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 	/* Now re-sort the routes in quality order */
 	switch (nr_node->count) {
 	case 3:
-		if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
-			switch (nr_node->which) {
-			case 0:
-				nr_node->which = 1;
-				break;
-			case 1:
-				nr_node->which = 0;
-				break;
-			}
-			nr_route           = nr_node->routes[0];
-			nr_node->routes[0] = nr_node->routes[1];
-			nr_node->routes[1] = nr_route;
-		}
-		if (nr_node->routes[2].quality > nr_node->routes[1].quality) {
-			switch (nr_node->which) {
-			case 1:  nr_node->which = 2;
-				break;
-
-			case 2:  nr_node->which = 1;
-				break;
-
-			default:
-				break;
-			}
-			nr_route           = nr_node->routes[1];
-			nr_node->routes[1] = nr_node->routes[2];
-			nr_node->routes[2] = nr_route;
-		}
+		re_sort_routes(nr_node, 0, 1);
+		re_sort_routes(nr_node, 1, 2);
 	case 2:
-		if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
-			switch (nr_node->which) {
-			case 0:  nr_node->which = 1;
-				break;
-
-			case 1:  nr_node->which = 0;
-				break;
-
-			default: break;
-			}
-			nr_route           = nr_node->routes[0];
-			nr_node->routes[0] = nr_node->routes[1];
-			nr_node->routes[1] = nr_route;
-			}
+		re_sort_routes(nr_node, 0, 1);
 	case 1:
 		break;
 	}
-- 
2.7.4

[PATCH v3 2/2] net: netrom: nr_route: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
 None.

Changes in v3:
 Update subject.

 net/netrom/nr_route.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index fba4b4c..75e6ba9 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -265,6 +265,7 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 	case 3:
 		re_sort_routes(nr_node, 0, 1);
 		re_sort_routes(nr_node, 1, 2);
+		/* fall through */
 	case 2:
 		re_sort_routes(nr_node, 0, 1);
 	case 1:
@@ -357,6 +358,7 @@ static int nr_del_node(ax25_address *callsign, ax25_address *neighbour, struct n
 				switch (i) {
 				case 0:
 					nr_node->routes[0] = nr_node->routes[1];
+					/* fall through */
 				case 1:
 					nr_node->routes[1] = nr_node->routes[2];
 				case 2:
@@ -526,6 +528,7 @@ void nr_rt_device_down(struct net_device *dev)
 						switch (i) {
 						case 0:
 							t->routes[0] = t->routes[1];
+							/* fall through */
 						case 1:
 							t->routes[1] = t->routes[2];
 						case 2:
-- 
2.7.4

Re: [PATCH v3 0/2] refactor code and mark expected switch fall-throughs

[David Miller]

From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date: Fri, 27 Oct 2017 00:50:57 -0500

> The aim of this patchset is firstly to refactor code in nr_route.c in order to make it
> easier to read and maintain and, secondly, to mark some expected switch fall-throughs
> in preparation to enabling -Wimplicit-fallthrough.
> 
> I have to mention that I did not implement any unit test.
> If someone has any suggestions on how I could test this piece of code
> it'd be greatly appreciated.

Series applied, thank you.

Re: [PATCH v3 0/2] refactor code and mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting David Miller <davem@davemloft.net>:

> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Fri, 27 Oct 2017 00:50:57 -0500
>
>> The aim of this patchset is firstly to refactor code in nr_route.c  
>> in order to make it
>> easier to read and maintain and, secondly, to mark some expected  
>> switch fall-throughs
>> in preparation to enabling -Wimplicit-fallthrough.
>>
>> I have to mention that I did not implement any unit test.
>> If someone has any suggestions on how I could test this piece of code
>> it'd be greatly appreciated.
>
> Series applied, thank you.

Glad to help.

Thanks
--
Gustavo A. R. Silva