[spp] [PATCH 0/6] Fix bug of assigning ports

[spp] [PATCH 0/6] Fix bug of assigning ports

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

This series of patches is to fix the error for adding, deleting and
patching incorrect ports which is cannot craete or does not exist. To
fix the bug, correct determining the existing port with
parse_resource_uid(). One of the reasons for the bug is that is
imcomplete to catch up with changing to uint16_t for port_id and Some of
checking port_id are incorrect.

This update is to correct the bug of managing ports, and also refactor
log messages for add, del and patch commands.

Yasufumi Ogawa (6):
  spp_nfv: fix bug of do_del if port does not exist
  spp_nfv: refactor log of patch command
  spp_vm: fix bug of do_del if port does not exist
  spp_vm: fix to use uint16_t port_id in do_add
  spp_vm: correct error handling if patch cmd failed
  spp_vm: refactor log of patch command

 src/nfv/nfv.c |  98 ++++++++++++++++++++++++++++++++++++++++----------------
 src/vm/main.c | 101 ++++++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 144 insertions(+), 55 deletions(-)

-- 
2.7.4

[spp] [PATCH 1/6] spp_nfv: fix bug of do_del if port does not exist

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

Fix bug of do_del() causing segmentation fault if not existing port is
deleted. It is because determining the result of find_port_id() is
incorrect and tries to delete invalid port.

This update is to correct the result of find_port_id() and determining
port ID.

It is also including refactor of patch command in which find_port_id()
is used. An error message is shown in log without terminating the
process if patched port is not found.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 src/nfv/nfv.c | 80 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 22 deletions(-)

diff --git a/src/nfv/nfv.c b/src/nfv/nfv.c
index c261e8d..049b3cf 100644
--- a/src/nfv/nfv.c
+++ b/src/nfv/nfv.c
@@ -265,11 +265,15 @@ forward_array_remove(int port_id)
 	}
 }
 
-static int
+/*
+ * Return actual port ID which is assigned by system internally, or PORT_RESET
+ * if port is not found.
+ */
+static uint16_t
 find_port_id(int id, enum port_type type)
 {
-	int port_id = PORT_RESET;
-	unsigned int i;
+	uint16_t port_id = PORT_RESET;
+	uint16_t i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (port_map[i].port_type != type)
@@ -287,18 +291,21 @@ find_port_id(int id, enum port_type type)
 static int
 do_del(char *res_uid)
 {
-	int port_id = PORT_RESET;
+	uint16_t port_id = PORT_RESET;
 	char *p_type;
 	int p_id;
 	int res;
 
 	res = parse_resource_uid(res_uid, &p_type, &p_id);
-	if (res < 0)
+	if (res < 0) {
+		RTE_LOG(ERR, APP,
+			"Failed to parse resource UID\n");
 		return -1;
+	}
 
 	if (!strcmp(p_type, "vhost")) {
 		port_id = find_port_id(p_id, VHOST);
-		if (port_id < 0)
+		if (port_id == PORT_RESET)
 			return -1;
 
 	} else if (!strcmp(p_type, "ring")) {
@@ -306,7 +313,7 @@ do_del(char *res_uid)
 
 		RTE_LOG(DEBUG, APP, "Del ring id %d\n", p_id);
 		port_id = find_port_id(p_id, RING);
-		if (port_id < 0)
+		if (port_id == PORT_RESET)
 			return -1;
 
 		rte_eth_dev_detach(port_id, name);
@@ -315,7 +322,7 @@ do_del(char *res_uid)
 		char name[RTE_ETH_NAME_MAX_LEN];
 
 		port_id = find_port_id(p_id, PCAP);
-		if (port_id < 0)
+		if (port_id == PORT_RESET)
 			return -1;
 
 		rte_eth_dev_detach(port_id, name);
@@ -324,7 +331,7 @@ do_del(char *res_uid)
 		char name[RTE_ETH_NAME_MAX_LEN];
 
 		port_id = find_port_id(p_id, NULLPMD);
-		if (port_id < 0)
+		if (port_id == PORT_RESET)
 			return -1;
 
 		rte_eth_dev_detach(port_id, name);
@@ -694,7 +701,7 @@ do_add(char *res_uid)
 	else
 		port_id = (uint16_t) res;
 
-	port_map[port_id].id = (uint16_t) p_id;
+	port_map[port_id].id = p_id;
 	port_map[port_id].port_type = type;
 	port_map[port_id].stats = &ports->client_stats[p_id];
 
@@ -775,23 +782,52 @@ parse_command(char *str)
 			/* reset forward array*/
 			forward_array_reset();
 		} else {
-			int in_port;
-			int out_port;
+			uint16_t in_port;
+			uint16_t out_port;
 
 			if (max_token <= 2)
 				return 0;
 
-			char *p_type;
-			int p_id;
-
-			parse_resource_uid(token_list[1], &p_type, &p_id);
-			in_port = find_port_id(p_id, get_port_type(p_type));
-
-			parse_resource_uid(token_list[2], &p_type, &p_id);
-			out_port = find_port_id(p_id, get_port_type(p_type));
-
-			if (in_port < 0 || out_port < 0)
+			char *in_p_type;
+			char *out_p_type;
+			int in_p_id;
+			int out_p_id;
+
+			parse_resource_uid(token_list[1], &in_p_type, &in_p_id);
+			in_port = find_port_id(in_p_id,
+					get_port_type(in_p_type));
+
+			parse_resource_uid(token_list[2],
+					&out_p_type, &out_p_id);
+			out_port = find_port_id(out_p_id,
+					get_port_type(out_p_type));
+
+			if (in_port == PORT_RESET && out_port == PORT_RESET) {
+				char err_msg[128];
+				memset(err_msg, '\0', sizeof(err_msg));
+				sprintf(err_msg, "%s '%s:%d' and '%s:%d' %s",
+					"Failed to patch, both of",
+					in_p_type, in_p_id,
+					out_p_type, out_p_id,
+					"not found");
+				RTE_LOG(ERR, APP, "%s\n", err_msg);
+				return 0;
+			} else if (in_port == PORT_RESET) {
+				char err_msg[128];
+				memset(err_msg, '\0', sizeof(err_msg));
+				sprintf(err_msg, "%s '%s:%d' not found",
+					"Failed to patch, in_port",
+					in_p_type, in_p_id);
+				RTE_LOG(ERR, APP, "%s\n", err_msg);
 				return 0;
+			} else if (out_port == PORT_RESET) {
+				char err_msg[128];
+				memset(err_msg, '\0', sizeof(err_msg));
+				sprintf(err_msg, "%s '%s:%d' not found",
+					"Failed to patch, out_port",
+					out_p_type, out_p_id);
+				RTE_LOG(ERR, APP, "%s\n", err_msg);
+			}
 
 			add_patch(in_port, out_port);
 		}
-- 
2.7.4

[spp] [PATCH 2/6] spp_nfv: refactor log of patch command

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

To describe the result of patch command correctly, refactor log message.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 src/nfv/nfv.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/src/nfv/nfv.c b/src/nfv/nfv.c
index 049b3cf..05290ed 100644
--- a/src/nfv/nfv.c
+++ b/src/nfv/nfv.c
@@ -344,17 +344,19 @@ do_del(char *res_uid)
 	return 0;
 }
 
+/* Return 0 if invalid */
 static int
-is_valid_port(int port_id)
+is_valid_port(uint16_t port_id)
 {
-	if (port_id < 0 || port_id > RTE_MAX_ETHPORTS)
+	if (port_id > RTE_MAX_ETHPORTS)
 		return 0;
 
 	return port_map[port_id].id != PORT_RESET;
 }
 
+/* Return -1 as an error if given patch is invalid */
 static int
-add_patch(int in_port, int out_port)
+add_patch(uint16_t in_port, uint16_t out_port)
 {
 	if (!is_valid_port(in_port) || !is_valid_port(out_port))
 		return -1;
@@ -711,6 +713,7 @@ do_add(char *res_uid)
 	return 0;
 }
 
+/* Return -1 if exit command is called to terminate the process */
 static int
 parse_command(char *str)
 {
@@ -805,31 +808,36 @@ parse_command(char *str)
 			if (in_port == PORT_RESET && out_port == PORT_RESET) {
 				char err_msg[128];
 				memset(err_msg, '\0', sizeof(err_msg));
-				sprintf(err_msg, "%s '%s:%d' and '%s:%d' %s",
-					"Failed to patch, both of",
+				sprintf(err_msg, "%s '%s:%d' and '%s:%d'",
+					"Patch not found, both of",
 					in_p_type, in_p_id,
-					out_p_type, out_p_id,
-					"not found");
+					out_p_type, out_p_id);
 				RTE_LOG(ERR, APP, "%s\n", err_msg);
-				return 0;
 			} else if (in_port == PORT_RESET) {
 				char err_msg[128];
 				memset(err_msg, '\0', sizeof(err_msg));
-				sprintf(err_msg, "%s '%s:%d' not found",
-					"Failed to patch, in_port",
+				sprintf(err_msg, "%s '%s:%d'",
+					"Patch not found, in_port",
 					in_p_type, in_p_id);
 				RTE_LOG(ERR, APP, "%s\n", err_msg);
-				return 0;
 			} else if (out_port == PORT_RESET) {
 				char err_msg[128];
 				memset(err_msg, '\0', sizeof(err_msg));
-				sprintf(err_msg, "%s '%s:%d' not found",
-					"Failed to patch, out_port",
+				sprintf(err_msg, "%s '%s:%d'",
+					"Patch not found, out_port",
 					out_p_type, out_p_id);
 				RTE_LOG(ERR, APP, "%s\n", err_msg);
 			}
 
-			add_patch(in_port, out_port);
+			if (add_patch(in_port, out_port) == 0)
+				RTE_LOG(INFO, APP,
+					"Patched '%s:%d' and '%s:%d'\n",
+					in_p_type, in_p_id,
+					out_p_type, out_p_id);
+
+			else
+				RTE_LOG(ERR, APP, "Failed to patch\n");
+			ret = 0;
 		}
 
 	} else if (!strcmp(token_list[0], "del")) {
@@ -993,7 +1001,7 @@ main(int argc, char *argv[])
 		RTE_LOG(DEBUG, APP, "Received string: %s\n", str);
 
 		ret = parse_command(str);
-		if (ret < 0)
+		if (ret < 0)  /* terminate process if exit is called */
 			break;
 
 		/*Send the message back to client*/
-- 
2.7.4

[spp] [PATCH 3/6] spp_vm: fix bug of do_del if port does not exist

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

Fix bug of do_del() causing segmentation fault if not existing port is
deleted. It is because determining the result of find_port_id() is
incorrect and tries to delete invalid port.

This update is to correct the result of find_port_id() and determining
port ID.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 src/vm/main.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/vm/main.c b/src/vm/main.c
index 63d5684..3807370 100644
--- a/src/vm/main.c
+++ b/src/vm/main.c
@@ -205,11 +205,15 @@ forward_array_remove(int port_id)
 	}
 }
 
-static int
+/*
+ * Return actual port ID which is assigned by system internally, or PORT_RESET
+ * if port is not found.
+ */
+static uint16_t
 find_port_id(int id, enum port_type type)
 {
-	int port_id = PORT_RESET;
-	unsigned int i;
+	uint16_t port_id = PORT_RESET;
+	uint16_t i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (port_map[i].port_type != type)
@@ -227,21 +231,24 @@ find_port_id(int id, enum port_type type)
 static int
 do_del(char *res_uid)
 {
-	int port_id = PORT_RESET;
+	uint16_t port_id = PORT_RESET;
 	char *p_type;
 	int p_id;
 	int res;
 
 	res = parse_resource_uid(res_uid, &p_type, &p_id);
-	if (res < 0)
+	if (res < 0) {
+		RTE_LOG(ERR, APP,
+			"Failed to parse resource UID\n");
 		return -1;
+	}
 
 	if (!strcmp(p_type, "ring")) {
 		char name[RTE_ETH_NAME_MAX_LEN];
 
 		RTE_LOG(DEBUG, APP, "Del ring id %d\n", p_id);
 		port_id = find_port_id(p_id, RING);
-		if (port_id < 0)
+		if (port_id == PORT_RESET)
 			return -1;
 
 		rte_eth_dev_detach(port_id, name);
-- 
2.7.4

[spp] [PATCH 4/6] spp_vm: fix to use uint16_t port_id in do_add

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

Modify to use uint16_t type for port_id.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 src/vm/main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/vm/main.c b/src/vm/main.c
index 3807370..2813fca 100644
--- a/src/vm/main.c
+++ b/src/vm/main.c
@@ -373,7 +373,7 @@ static int
 do_add(char *res_uid)
 {
 	enum port_type type = UNDEF;
-	int port_id = PORT_RESET;
+	uint16_t port_id = PORT_RESET;
 	char *p_type;
 	int p_id;
 	int res;
@@ -384,11 +384,13 @@ do_add(char *res_uid)
 
 	if (!strcmp(p_type, "ring")) {
 		type = RING;
-		port_id = add_ring_pmd(p_id);
+		res = add_ring_pmd(p_id);
 	}
 
-	if (port_id < 0)
+	if (res < 0)
 		return -1;
+	else
+		port_id = (uint16_t) res;
 
 	port_map[port_id].id = p_id;
 	port_map[port_id].port_type = type;
-- 
2.7.4

[spp] [PATCH 5/6] spp_vm: correct error handling if patch cmd failed

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

Refactor of patch command in which find_port_id() is used. This update
is to show an error message in log without terminating the process if
patched port is not found.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 src/vm/main.c | 54 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/src/vm/main.c b/src/vm/main.c
index 2813fca..997db5b 100644
--- a/src/vm/main.c
+++ b/src/vm/main.c
@@ -402,6 +402,7 @@ do_add(char *res_uid)
 	return 0;
 }
 
+/* Return -1 if exit command is called to terminate the process */
 static int
 parse_command(char *str)
 {
@@ -473,23 +474,52 @@ parse_command(char *str)
 			/* reset forward array*/
 			forward_array_reset();
 		} else {
-			int in_port;
-			int out_port;
+			uint16_t in_port;
+			uint16_t out_port;
 
 			if (max_token <= 2)
 				return 0;
 
-			char *p_type;
-			int p_id;
-
-			parse_resource_uid(token_list[1], &p_type, &p_id);
-			in_port = find_port_id(p_id, get_port_type(p_type));
-
-			parse_resource_uid(token_list[2], &p_type, &p_id);
-			out_port = find_port_id(p_id, get_port_type(p_type));
-
-			if (in_port < 0 || out_port < 0)
+			char *in_p_type;
+			char *out_p_type;
+			int in_p_id;
+			int out_p_id;
+
+			parse_resource_uid(token_list[1], &in_p_type, &in_p_id);
+			in_port = find_port_id(in_p_id,
+					get_port_type(in_p_type));
+
+			parse_resource_uid(token_list[2],
+					&out_p_type, &out_p_id);
+			out_port = find_port_id(out_p_id,
+					get_port_type(out_p_type));
+
+			if (in_port == PORT_RESET && out_port == PORT_RESET) {
+				char err_msg[128];
+				memset(err_msg, '\0', sizeof(err_msg));
+				sprintf(err_msg, "%s '%s:%d' and '%s:%d' %s",
+						"Failed to patch, both of",
+						in_p_type, in_p_id,
+						out_p_type, out_p_id,
+						"not found");
+				RTE_LOG(ERR, APP, "%s\n", err_msg);
+				return 0;
+			} else if (in_port == PORT_RESET) {
+				char err_msg[128];
+				memset(err_msg, '\0', sizeof(err_msg));
+				sprintf(err_msg, "%s '%s:%d' not found",
+						"Failed to patch, in_port",
+						in_p_type, in_p_id);
+				RTE_LOG(ERR, APP, "%s\n", err_msg);
 				return 0;
+			} else if (out_port == PORT_RESET) {
+				char err_msg[128];
+				memset(err_msg, '\0', sizeof(err_msg));
+				sprintf(err_msg, "%s '%s:%d' not found",
+						"Failed to patch, out_port",
+						out_p_type, out_p_id);
+				RTE_LOG(ERR, APP, "%s\n", err_msg);
+			}
 
 			add_patch(in_port, out_port);
 		}
-- 
2.7.4

[spp] [PATCH 6/6] spp_vm: refactor log of patch command

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

To describe the result of patch command correctly, refactor log message.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 src/vm/main.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/vm/main.c b/src/vm/main.c
index 997db5b..1d7d83a 100644
--- a/src/vm/main.c
+++ b/src/vm/main.c
@@ -260,17 +260,19 @@ do_del(char *res_uid)
 	return 0;
 }
 
+/* Return 0 if invalid */
 static int
-is_valid_port(int port_id)
+is_valid_port(uint16_t port_id)
 {
-	if (port_id < 0 || port_id > RTE_MAX_ETHPORTS)
+	if (port_id > RTE_MAX_ETHPORTS)
 		return 0;
 
 	return port_map[port_id].id != PORT_RESET;
 }
 
+/* Return -1 as an error if given patch is invalid */
 static int
-add_patch(int in_port, int out_port)
+add_patch(uint16_t in_port, uint16_t out_port)
 {
 	if (!is_valid_port(in_port) || !is_valid_port(out_port))
 		return -1;
@@ -497,31 +499,35 @@ parse_command(char *str)
 			if (in_port == PORT_RESET && out_port == PORT_RESET) {
 				char err_msg[128];
 				memset(err_msg, '\0', sizeof(err_msg));
-				sprintf(err_msg, "%s '%s:%d' and '%s:%d' %s",
-						"Failed to patch, both of",
+				sprintf(err_msg, "%s '%s:%d' and '%s:%d'",
+						"Patch not found, both of",
 						in_p_type, in_p_id,
-						out_p_type, out_p_id,
-						"not found");
+						out_p_type, out_p_id);
 				RTE_LOG(ERR, APP, "%s\n", err_msg);
-				return 0;
 			} else if (in_port == PORT_RESET) {
 				char err_msg[128];
 				memset(err_msg, '\0', sizeof(err_msg));
-				sprintf(err_msg, "%s '%s:%d' not found",
-						"Failed to patch, in_port",
+				sprintf(err_msg, "%s '%s:%d'",
+						"Patch not found, in_port",
 						in_p_type, in_p_id);
 				RTE_LOG(ERR, APP, "%s\n", err_msg);
-				return 0;
 			} else if (out_port == PORT_RESET) {
 				char err_msg[128];
 				memset(err_msg, '\0', sizeof(err_msg));
-				sprintf(err_msg, "%s '%s:%d' not found",
-						"Failed to patch, out_port",
+				sprintf(err_msg, "%s '%s:%d'",
+						"Patch not found, out_port",
 						out_p_type, out_p_id);
 				RTE_LOG(ERR, APP, "%s\n", err_msg);
 			}
 
-			add_patch(in_port, out_port);
+			if (add_patch(in_port, out_port) == 0)
+				RTE_LOG(INFO, APP,
+					"Patched '%s:%d' and '%s:%d'\n",
+					in_p_type, in_p_id,
+					out_p_type, out_p_id);
+			else
+				RTE_LOG(ERR, APP, "Failed to patch\n");
+			ret = 0;
 		}
 	} else if (strncmp(str, "del", 3) == 0) {
 		RTE_LOG(DEBUG, APP, "Received del command\n");
@@ -646,7 +652,7 @@ main(int argc, char *argv[])
 		RTE_LOG(DEBUG, APP, "Received string: %s\n", str);
 
 		ret = parse_command(str);
-		if (ret < 0)
+		if (ret < 0)  /* terminate process if exit is called */
 			break;
 
 		/*Send the message back to client*/
-- 
2.7.4