[spp] [PATCH 0/5] Add error handling for add and del commands

[spp] [PATCH 0/5] Add error handling for add and del commands

[ogawa.yasufumi]

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

There is no strict checking for returned value of parsing resource UID
for add and del commands. It might cause a fatal error if no existing
resource is accessed for inappropriate resource UID.

To avoid inappropriate accessing, add checking for the resource UID and
correct determining of the returned value. This update also includes
refactors of log messages in add_ring_pmd() to create a ring port
correctly.

Yasufumi Ogawa (5):
  shared: add error handling for invalid res UID
  spp_nfv: add error handling for add and del cmd
  spp_vm: add error handling for add and del cmd
  spp_nfv: refactor add_ring_pmd
  spp_vm: refactor add_ring_pmd

 src/nfv/nfv.c       | 66 ++++++++++++++++++++++++++++++++++++-----------------
 src/shared/common.c |  7 ++++++
 src/vm/main.c       | 57 +++++++++++++++++++++++++++++++--------------
 3 files changed, 92 insertions(+), 38 deletions(-)

-- 
2.7.4

[spp] [PATCH 1/5] shared: add error handling for invalid res UID

[ogawa.yasufumi]

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

There is no checking for the format of resource UID. It is caused
segmentation fault if invalid resouce UID is given to
parse_resource_uid(). This update is to add the validation and error
handling.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 src/shared/common.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/shared/common.c b/src/shared/common.c
index c78ee7b..bf776ee 100644
--- a/src/shared/common.c
+++ b/src/shared/common.c
@@ -228,6 +228,13 @@ parse_resource_uid(char *str, char **port_type, int *port_id)
 	char delim[] = ":";
 	char *endp;
 
+	RTE_LOG(DEBUG, APP, "Parsing resource UID: '%s\n'", str);
+	if (strstr(str, delim) == NULL) {
+		RTE_LOG(ERR, APP, "Invalid resource UID: '%s'\n", str);
+		return -1;
+	}
+	RTE_LOG(DEBUG, APP, "Delimiter %s is included\n", delim);
+
 	*port_type = strtok(str, delim);
 
 	token = strtok(NULL, delim);
-- 
2.7.4

[spp] [PATCH 2/5] spp_nfv: add error handling for add and del cmd

[ogawa.yasufumi]

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

Add error handling in do_add() and do_del() to avoid segmentation fault
caused if parse_resource_uid() is falied.

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

diff --git a/src/nfv/nfv.c b/src/nfv/nfv.c
index 931269d..e245a26 100644
--- a/src/nfv/nfv.c
+++ b/src/nfv/nfv.c
@@ -290,8 +290,11 @@ do_del(char *res_uid)
 	int port_id = PORT_RESET;
 	char *p_type;
 	int p_id;
+	int res;
 
-	parse_resource_uid(res_uid, &p_type, &p_id);
+	res = parse_resource_uid(res_uid, &p_type, &p_id);
+	if (res < 0)
+		return -1;
 
 	if (!strcmp(p_type, "vhost")) {
 		port_id = find_port_id(p_id, VHOST);
@@ -647,11 +650,13 @@ do_add(char *res_uid)
 {
 	enum port_type type = UNDEF;
 	int port_id = PORT_RESET;
-
 	char *p_type;
 	int p_id;
+	int res;
 
-	parse_resource_uid(res_uid, &p_type, &p_id);
+	res = parse_resource_uid(res_uid, &p_type, &p_id);
+	if (res < 0)
+		return -1;
 
 	if (!strcmp(p_type, "vhost")) {
 		type = VHOST;
@@ -740,8 +745,9 @@ parse_command(char *str)
 		cmd = FORWARD;
 
 	} else if (!strcmp(token_list[0], "add")) {
-		RTE_LOG(DEBUG, APP, "add\n");
-		do_add(token_list[1]);
+		RTE_LOG(DEBUG, APP, "Received add command\n");
+		if (do_add(token_list[1]) < 0)
+			RTE_LOG(ERR, APP, "Failed to do_add()\n");
 
 	} else if (!strcmp(token_list[0], "patch")) {
 		RTE_LOG(DEBUG, APP, "patch\n");
@@ -775,10 +781,12 @@ parse_command(char *str)
 		}
 
 	} else if (!strcmp(token_list[0], "del")) {
-		RTE_LOG(DEBUG, APP, "del\n");
+		RTE_LOG(DEBUG, APP, "Received del command\n");
 
 		cmd = STOP;
-		do_del(token_list[1]);
+
+		if (do_del(token_list[1]) < 0)
+			RTE_LOG(ERR, APP, "Failed to do_del()\n");
 	}
 
 	return ret;
-- 
2.7.4

[spp] [PATCH 3/5] spp_vm: add error handling for add and del cmd

[ogawa.yasufumi]

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

Add error handling in do_add() and do_del() to avoid segmentation fault
caused if parse_resource_uid() is falied.

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

diff --git a/src/vm/main.c b/src/vm/main.c
index 7aec126..55592df 100644
--- a/src/vm/main.c
+++ b/src/vm/main.c
@@ -230,8 +230,11 @@ do_del(char *res_uid)
 	int port_id = PORT_RESET;
 	char *p_type;
 	int p_id;
+	int res;
 
-	parse_resource_uid(res_uid, &p_type, &p_id);
+	res = parse_resource_uid(res_uid, &p_type, &p_id);
+	if (res < 0)
+		return -1;
 
 	if (!strcmp(p_type, "ring")) {
 		char name[RTE_ETH_NAME_MAX_LEN];
@@ -349,11 +352,13 @@ do_add(char *res_uid)
 {
 	enum port_type type = UNDEF;
 	int port_id = PORT_RESET;
-
 	char *p_type;
 	int p_id;
+	int res;
 
-	parse_resource_uid(res_uid, &p_type, &p_id);
+	res = parse_resource_uid(res_uid, &p_type, &p_id);
+	if (res < 0)
+		return -1;
 
 	if (!strcmp(p_type, "ring")) {
 		type = RING;
@@ -430,8 +435,9 @@ parse_command(char *str)
 		cmd = FORWARD;
 
 	} else if (strncmp(token_list[0], "add", 3) == 0) {
-		RTE_LOG(DEBUG, APP, "add\n");
-		do_add(token_list[1]);
+		RTE_LOG(DEBUG, APP, "Received add command\n");
+		if (do_add(token_list[1]) < 0)
+			RTE_LOG(ERR, APP, "Failed to do_add()\n");
 
 	} else if (!strcmp(token_list[0], "patch")) {
 		RTE_LOG(DEBUG, APP, "patch\n");
@@ -464,10 +470,12 @@ parse_command(char *str)
 			add_patch(in_port, out_port);
 		}
 	} else if (strncmp(str, "del", 3) == 0) {
-		RTE_LOG(DEBUG, APP, "del\n");
+		RTE_LOG(DEBUG, APP, "Received del command\n");
 
 		cmd = STOP;
-		do_del(token_list[1]);
+
+		if (do_del(token_list[1]) < 0)
+			RTE_LOG(ERR, APP, "Failed to do_del()\n");
 	}
 
 	return ret;
-- 
2.7.4

[spp] [PATCH 4/5] spp_nfv: refactor add_ring_pmd

[ogawa.yasufumi]

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

* Correct assigning of uint16_to for port ID which is assigned as int
  incorrectly.

* Correct log messages.

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

diff --git a/src/nfv/nfv.c b/src/nfv/nfv.c
index e245a26..c261e8d 100644
--- a/src/nfv/nfv.c
+++ b/src/nfv/nfv.c
@@ -373,25 +373,39 @@ add_patch(int in_port, int out_port)
 	return 0;
 }
 
+/*
+ * Create ring PMD with given ring_id.
+ */
 static int
 add_ring_pmd(int ring_id)
 {
 	struct rte_ring *ring;
-	int ring_port_id;
+	int res;
+	char rx_queue_name[32];  /* Prefix and number like as 'eth_ring_0' */
 
-	/* look up ring, based on user's provided id*/
-	ring = rte_ring_lookup(get_rx_queue_name(ring_id));
+	memset(rx_queue_name, '\0', sizeof(rx_queue_name));
+	sprintf(rx_queue_name, "%s", get_rx_queue_name(ring_id));
+
+	/* Look up ring with provided ring_id */
+	ring = rte_ring_lookup(rx_queue_name);
 	if (ring == NULL) {
 		RTE_LOG(ERR, APP,
-			"Cannot get RX ring - is server process running?\n");
+			"Failed to get RX ring %s - is primary running?\n",
+			rx_queue_name);
 		return -1;
 	}
+	RTE_LOG(INFO, APP, "Looked up ring '%s'\n", rx_queue_name);
 
 	/* create ring pmd*/
-	ring_port_id = rte_eth_from_ring(ring);
-	RTE_LOG(DEBUG, APP, "ring port id %d\n", ring_port_id);
+	res = rte_eth_from_ring(ring);
+	if (res < 0) {
+		RTE_LOG(ERR, APP,
+			"Cannot create eth dev with rte_eth_from_ring()\n");
+		return -1;
+	}
+	RTE_LOG(INFO, APP, "Created ring PMD: %d\n", res);
 
-	return ring_port_id;
+	return res;
 }
 
 static int
@@ -649,7 +663,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;
@@ -660,25 +674,27 @@ do_add(char *res_uid)
 
 	if (!strcmp(p_type, "vhost")) {
 		type = VHOST;
-		port_id = add_vhost_pmd(p_id);
+		res = add_vhost_pmd(p_id);
 
 	} else if (!strcmp(p_type, "ring")) {
 		type = RING;
-		port_id = add_ring_pmd(p_id);
+		res = add_ring_pmd(p_id);
 
 	} else if (!strcmp(p_type, "pcap")) {
 		type = PCAP;
-		port_id = add_pcap_pmd(p_id);
+		res = add_pcap_pmd(p_id);
 
 	} else if (!strcmp(p_type, "nullpmd")) {
 		type = NULLPMD;
-		port_id = add_null_pmd(p_id);
+		res = add_null_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].id = (uint16_t) p_id;
 	port_map[port_id].port_type = type;
 	port_map[port_id].stats = &ports->client_stats[p_id];
 
-- 
2.7.4

[spp] [PATCH 5/5] spp_vm: refactor add_ring_pmd

[ogawa.yasufumi]

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

* Correct assigning of uint16_to for port ID which is assigned as int
  incorrectly.

* Correct log messages.

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

diff --git a/src/vm/main.c b/src/vm/main.c
index 55592df..63d5684 100644
--- a/src/vm/main.c
+++ b/src/vm/main.c
@@ -315,32 +315,47 @@ get_memzone_by_addr(const void *addr)
 	return mz;
 }
 
+/*
+ * Create ring PMD with given ring_id.
+ */
 static int
 add_ring_pmd(int ring_id)
 {
 	const struct rte_memzone *memzone;
 	struct rte_ring *ring;
-	int ring_port_id;
+	int res;
+	char rx_queue_name[32];  /* Prefix and number like as 'eth_ring_0' */
 
-	/* look up ring, based on user's provided id*/
-	ring = rte_ring_lookup(get_rx_queue_name(ring_id));
+	memset(rx_queue_name, '\0', sizeof(rx_queue_name));
+	sprintf(rx_queue_name, "%s", get_rx_queue_name(ring_id));
+
+	/* Look up ring with provided ring_id */
+	ring = rte_ring_lookup(rx_queue_name);
 	if (ring == NULL) {
 		RTE_LOG(ERR, APP,
-			"Cannot get RX ring - is server process running?\n");
+			"Failed to get RX ring %s - is primary running?\n",
+			rx_queue_name);
 		return -1;
 	}
+	RTE_LOG(INFO, APP, "Looked up ring '%s'\n", rx_queue_name);
 
 	memzone = get_memzone_by_addr(ring);
-	if (memzone == NULL)
+	if (memzone == NULL) {
+		RTE_LOG(ERR, APP, "Cannot get memzone\n");
 		return -1;
+	}
 
-	/* create ring pmd*/
+	/* Create ring pmd */
 	ring->memzone = memzone;
-	ring_port_id = rte_eth_from_ring(ring);
-
-	RTE_LOG(DEBUG, APP, "ring port id %d\n", ring_port_id);
+	res = rte_eth_from_ring(ring);
+	if (res < 0) {
+		RTE_LOG(ERR, APP,
+			"Cannot create eth dev with rte_eth_from_ring()\n");
+		return -1;
+	}
+	RTE_LOG(INFO, APP, "Created ring PMD: %d\n", res);
 
-	return ring_port_id;
+	return res;
 }
 
 /**
-- 
2.7.4