[spp] [PATCH 0/6] Refactor parsing resource UID

[spp] [PATCH 0/6] Refactor parsing resource UID

[ogawa.yasufumi]

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

This series of patches is for fixing typo of func name, revising
ambiguous name of func and vars, and updating comments and log
messages.

Yasufumi Ogawa (6):
  shared/sec: refactor func for getting VLAN ID
  shared/sec: rename misspelled get_arrary_index
  shared/sec: remove misunderstandable validate func
  shared/sec: rename func for parsing port UID
  shared/sec: fix funcs for getting int and uint
  shared/sec: rename func for parsing lcore ID

 .../secondary/spp_worker_th/cmd_parser.c      | 108 +++++++++---------
 src/shared/secondary/spp_worker_th/spp_proc.h |   4 +-
 2 files changed, 55 insertions(+), 57 deletions(-)

-- 
2.17.1

[spp] [PATCH 1/6] shared/sec: refactor func for getting VLAN ID

[ogawa.yasufumi]

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

Functions for getting VLAN ID or PCAP are named as `get_int_value()`
and `get_uint_value()`. It is not describing the feature and should be
renamed. This update is to change the names to `get_vlan_int_val()` and
`get_vlan_uint_val()`, and revise comments for the functions.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 .../secondary/spp_worker_th/cmd_parser.c      | 35 +++++++++----------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index 1c069cc..cf7a7a6 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -219,36 +219,33 @@ get_arrary_index(const char *match, const char *list[])
 	return SPP_RET_NG;
 }
 
-/* Get int type value */
+/**
+ * Get VLAN ID or PCP as int from given val. It validates if the val is in the
+ * range from min to max given as third and fourth args.
+ */
 static int
-get_int_value(
-		int *output,
-		const char *arg_val,
-		int min,
-		int max)
+get_vlan_int_val(int *output, const char *arg_val, int min, int max)
 {
-	int ret = 0;
+	int ret;
 	char *endptr = NULL;
 	ret = strtol(arg_val, &endptr, 0);
 	if (unlikely(endptr == arg_val) || unlikely(*endptr != '\0'))
 		return SPP_RET_NG;
-
 	if (unlikely(ret < min) || unlikely(ret > max))
 		return SPP_RET_NG;
-
 	*output = ret;
 	return SPP_RET_OK;
 }
 
-/* Get unsigned int type value */
+/**
+ * Get VLAN ID or PCP as uint from given val. It validates if the val is in the
+ * range from min to max given as third and fourth args.
+ */
 static int
-get_uint_value(
-		unsigned int *output,
-		const char *arg_val,
-		unsigned int min,
+get_vlan_uint_val(unsigned int *output, const char *arg_val, unsigned int min,
 		unsigned int max)
 {
-	unsigned int ret = SPP_RET_OK;
+	unsigned int ret;
 	char *endptr = NULL;
 	ret = strtoul(arg_val, &endptr, 0);
 	if (unlikely(endptr == arg_val) || unlikely(*endptr != '\0'))
@@ -292,7 +289,7 @@ static int
 decode_core_value(void *output, const char *arg_val)
 {
 	int ret = SPP_RET_OK;
-	ret = get_uint_value(output, arg_val, 0, RTE_MAX_LCORE-1);
+	ret = get_vlan_uint_val(output, arg_val, 0, RTE_MAX_LCORE-1);
 	if (unlikely(ret < 0)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad core id. val=%s\n",
 				arg_val);
@@ -537,7 +534,7 @@ decode_port_vid(void *output, const char *arg_val,
 
 	switch (ability->ope) {
 	case SPP_PORT_ABILITY_OPE_ADD_VLANTAG:
-		ret = get_int_value(&ability->data.vlantag.vid,
+		ret = get_vlan_int_val(&ability->data.vlantag.vid,
 			arg_val, 0, ETH_VLAN_ID_MAX);
 		if (unlikely(ret < SPP_RET_OK)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC,
@@ -565,7 +562,7 @@ decode_port_pcp(void *output, const char *arg_val,
 
 	switch (ability->ope) {
 	case SPP_PORT_ABILITY_OPE_ADD_VLANTAG:
-		ret = get_int_value(&ability->data.vlantag.pcp,
+		ret = get_vlan_int_val(&ability->data.vlantag.pcp,
 				arg_val, 0, SPP_VLAN_PCP_MAX);
 		if (unlikely(ret < SPP_RET_OK)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC,
@@ -652,7 +649,7 @@ decode_classifier_vid_value(void *output, const char *arg_val,
 				int allow_override __attribute__ ((unused)))
 {
 	int ret = SPP_RET_NG;
-	ret = get_int_value(output, arg_val, 0, ETH_VLAN_ID_MAX);
+	ret = get_vlan_int_val(output, arg_val, 0, ETH_VLAN_ID_MAX);
 	if (unlikely(ret < SPP_RET_OK)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad VLAN ID. val=%s\n",
 				arg_val);
-- 
2.17.1

[spp] [PATCH 2/6] shared/sec: rename misspelled get_arrary_index

[ogawa.yasufumi]

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

The name of function `get_arrary_index()` of `arrary` is misspelled.
This update is to change the name to `get_list_idx()` and revise its
comment.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 .../secondary/spp_worker_th/cmd_parser.c       | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index cf7a7a6..4ee8773 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -207,13 +207,13 @@ split_cmd_params(char *string, int max, int *argc, char *argv[])
 	return SPP_RET_OK;
 }
 
-/* Get index of array */
+/* Get index of given str from list. */
 static int
-get_arrary_index(const char *match, const char *list[])
+get_list_idx(const char *str, const char *list[])
 {
 	int i;
 	for (i = 0; list[i][0] != '\0'; i++) {
-		if (strcmp(list[i], match) == 0)
+		if (strcmp(list[i], str) == 0)
 			return i;
 	}
 	return SPP_RET_NG;
@@ -305,7 +305,7 @@ decode_component_action_value(void *output, const char *arg_val,
 				int allow_override __attribute__ ((unused)))
 {
 	int ret = SPP_RET_OK;
-	ret = get_arrary_index(arg_val, CMD_ACT_LIST);
+	ret = get_list_idx(arg_val, CMD_ACT_LIST);
 	if (unlikely(ret <= 0)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC,
 				"Unknown component action. val=%s\n",
@@ -391,7 +391,7 @@ decode_port_action_value(void *output, const char *arg_val,
 				int allow_override __attribute__ ((unused)))
 {
 	int ret = SPP_RET_OK;
-	ret = get_arrary_index(arg_val, CMD_ACT_LIST);
+	ret = get_list_idx(arg_val, CMD_ACT_LIST);
 	if (unlikely(ret <= 0)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC,
 				"Unknown port action. val=%s\n",
@@ -451,7 +451,7 @@ decode_port_rxtx_value(void *output, const char *arg_val, int allow_override)
 	int ret = SPP_RET_OK;
 	struct sppwk_cmd_port *port = output;
 
-	ret = get_arrary_index(arg_val, PORT_DIR_LIST);
+	ret = get_list_idx(arg_val, PORT_DIR_LIST);
 	if (unlikely(ret <= 0)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC, "Unknown port rxtx. val=%s\n",
 				arg_val);
@@ -502,7 +502,7 @@ decode_port_vlan_operation(void *output, const char *arg_val,
 
 	switch (ability->ope) {
 	case SPP_PORT_ABILITY_OPE_NONE:
-		ret = get_arrary_index(arg_val, PORT_ABILITY_LIST);
+		ret = get_list_idx(arg_val, PORT_ABILITY_LIST);
 		if (unlikely(ret <= 0)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC,
 					"Unknown port ability. val=%s\n",
@@ -607,7 +607,7 @@ decode_classifier_action_value(void *output, const char *arg_val,
 				int allow_override __attribute__ ((unused)))
 {
 	int ret = SPP_RET_OK;
-	ret = get_arrary_index(arg_val, CMD_ACT_LIST);
+	ret = get_list_idx(arg_val, CMD_ACT_LIST);
 	if (unlikely(ret <= 0)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC, "Unknown port action. val=%s\n",
 				arg_val);
@@ -631,7 +631,7 @@ decode_classifier_type_value(void *output, const char *arg_val,
 				int allow_override __attribute__ ((unused)))
 {
 	int ret = SPP_RET_OK;
-	ret = get_arrary_index(arg_val, CLS_TYPE_LIST);
+	ret = get_list_idx(arg_val, CLS_TYPE_LIST);
 	if (unlikely(ret <= 0)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC,
 				"Unknown classifier type. val=%s\n",
-- 
2.17.1

[spp] [PATCH 3/6] shared/sec: remove misunderstandable validate func

[ogawa.yasufumi]

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

Function `decode_str_value()` is not for decoding, but just validate
length of given string. In addition, It just consists of four lines and
used from two statements. This update is to remove almost nouse
function.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 .../secondary/spp_worker_th/cmd_parser.c      | 23 ++++++++-----------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index 4ee8773..02528d6 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -258,17 +258,6 @@ get_vlan_uint_val(unsigned int *output, const char *arg_val, unsigned int min,
 	return SPP_RET_OK;
 }
 
-/* decoding procedure of string */
-static int
-decode_str_value(char *output, const char *arg_val)
-{
-	if (strlen(arg_val) >= SPPWK_VAL_BUFSZ)
-		return SPP_RET_NG;
-
-	strcpy(output, arg_val);
-	return SPP_RET_OK;
-}
-
 /* decoding procedure of port */
 static int
 decode_port_value(void *output, const char *arg_val)
@@ -344,7 +333,11 @@ decode_component_name_value(void *output, const char *arg_val,
 		}
 	}
 
-	return decode_str_value(component->name, arg_val);
+	if (strlen(arg_val) >= SPPWK_VAL_BUFSZ)
+		return SPP_RET_NG;
+
+	strcpy(component->name, arg_val);
+	return SPP_RET_OK;
 }
 
 /* decoding procedure of core id for component command */
@@ -488,7 +481,11 @@ decode_port_name_value(void *output, const char *arg_val,
 		return SPP_RET_NG;
 	}
 
-	return decode_str_value(output, arg_val);
+	if (strlen(arg_val) >= SPPWK_VAL_BUFSZ)
+		return SPP_RET_NG;
+
+	strcpy(output, arg_val);
+	return SPP_RET_OK;
 }
 
 /* decoding procedure of vlan operation for port command */
-- 
2.17.1

[spp] [PATCH 4/6] shared/sec: rename func for parsing port UID

[ogawa.yasufumi]

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

This update is to rename `decode_port_value()` to `parse_port_uid()`
because it is not for decoding but parsing resource UID of port such
as `phy:0`, and the term `port value` is ambiguous.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 .../secondary/spp_worker_th/cmd_parser.c      | 19 ++++++++++++-------
 src/shared/secondary/spp_worker_th/spp_proc.h |  4 ++--
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index 02528d6..5658893 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -258,18 +258,23 @@ get_vlan_uint_val(unsigned int *output, const char *arg_val, unsigned int min,
 	return SPP_RET_OK;
 }
 
-/* decoding procedure of port */
+/* Parse given res UID of port and init object of struct sppwk_port_idx. */
+/* TODO(yasufum) Confirm why 1st arg is not sppwk_port_idx, but void. */
+/**
+ * TODO(yasufum) Confirm why this func is required. Is it not enough to use
+ * parse_resource_uid() ?
+ */
 static int
-decode_port_value(void *output, const char *arg_val)
+parse_port_uid(void *output, const char *arg_val)
 {
-	int ret = SPP_RET_OK;
+	int ret;
 	struct sppwk_port_idx *port = output;
 	ret = parse_resource_uid(arg_val, &port->iface_type, &port->iface_no);
 	if (unlikely(ret != 0)) {
-		RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad port. val=%s\n", arg_val);
+		RTE_LOG(ERR, SPP_COMMAND_PROC,
+				"Invalid resource UID '%s'.\n", arg_val);
 		return SPP_RET_NG;
 	}
-
 	return SPP_RET_OK;
 }
 
@@ -412,7 +417,7 @@ decode_port_port_value(void *output, const char *arg_val, int allow_override)
 	struct sppwk_port_idx tmp_port;
 	struct sppwk_cmd_port *port = output;
 
-	ret = decode_port_value(&tmp_port, arg_val);
+	ret = parse_port_uid(&tmp_port, arg_val);
 	if (ret < SPP_RET_OK)
 		return SPP_RET_NG;
 
@@ -665,7 +670,7 @@ parse_cls_port(void *cls_cmd_attr, const char *arg_val,
 	struct sppwk_port_idx tmp_port;
 	int64_t mac_addr = 0;
 
-	ret = decode_port_value(&tmp_port, arg_val);
+	ret = parse_port_uid(&tmp_port, arg_val);
 	if (ret < SPP_RET_OK)
 		return SPP_RET_NG;
 
diff --git a/src/shared/secondary/spp_worker_th/spp_proc.h b/src/shared/secondary/spp_worker_th/spp_proc.h
index aeb8e92..8f9f11d 100644
--- a/src/shared/secondary/spp_worker_th/spp_proc.h
+++ b/src/shared/secondary/spp_worker_th/spp_proc.h
@@ -191,8 +191,8 @@ struct sppwk_cls_attrs {
  * attributions, use `sppwk_port_info` which has additional port params.
  */
 struct sppwk_port_idx {
-	enum port_type  iface_type; /**< phy, vhost or ring */
-	int             iface_no;
+	enum port_type iface_type;  /**< phy, vhost or ring. */
+	int iface_no;
 };
 
 /* Define detailed port params in addition to `sppwk_port_idx`. */
-- 
2.17.1

[spp] [PATCH 5/6] shared/sec: fix funcs for getting int and uint

[ogawa.yasufumi]

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

The name `get_uint_value()` was renamed as `get_vlan_uint_val()`, but
it is used lcore ID. `get_int_value()` was also renamed as
`get_vlan_int_val()` which is used for getting VLAN ID, but can be used
for other purposes. The term `vlan` should be removed from these funcs.
In addition, the names do not contain that checking the given value is
in the given range. This update is to fix the issues.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 .../secondary/spp_worker_th/cmd_parser.c      | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index 5658893..e0da244 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -220,11 +220,11 @@ get_list_idx(const char *str, const char *list[])
 }
 
 /**
- * Get VLAN ID or PCP as int from given val. It validates if the val is in the
- * range from min to max given as third and fourth args.
+ * Get int from given val. It validates if the val is in the range from min to
+ * max given as third and fourth args. It is intended to get VLAN ID or PCP.
  */
 static int
-get_vlan_int_val(int *output, const char *arg_val, int min, int max)
+get_int_in_range(int *output, const char *arg_val, int min, int max)
 {
 	int ret;
 	char *endptr = NULL;
@@ -238,11 +238,11 @@ get_vlan_int_val(int *output, const char *arg_val, int min, int max)
 }
 
 /**
- * Get VLAN ID or PCP as uint from given val. It validates if the val is in the
- * range from min to max given as third and fourth args.
+ * Get uint from given val. It validates if the val is in the range from min to
+ * max given as third and fourth args. It is intended to get lcore ID.
  */
 static int
-get_vlan_uint_val(unsigned int *output, const char *arg_val, unsigned int min,
+get_uint_in_range(unsigned int *output, const char *arg_val, unsigned int min,
 		unsigned int max)
 {
 	unsigned int ret;
@@ -283,7 +283,7 @@ static int
 decode_core_value(void *output, const char *arg_val)
 {
 	int ret = SPP_RET_OK;
-	ret = get_vlan_uint_val(output, arg_val, 0, RTE_MAX_LCORE-1);
+	ret = get_uint_in_range(output, arg_val, 0, RTE_MAX_LCORE-1);
 	if (unlikely(ret < 0)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad core id. val=%s\n",
 				arg_val);
@@ -536,7 +536,7 @@ decode_port_vid(void *output, const char *arg_val,
 
 	switch (ability->ope) {
 	case SPP_PORT_ABILITY_OPE_ADD_VLANTAG:
-		ret = get_vlan_int_val(&ability->data.vlantag.vid,
+		ret = get_int_in_range(&ability->data.vlantag.vid,
 			arg_val, 0, ETH_VLAN_ID_MAX);
 		if (unlikely(ret < SPP_RET_OK)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC,
@@ -564,7 +564,7 @@ decode_port_pcp(void *output, const char *arg_val,
 
 	switch (ability->ope) {
 	case SPP_PORT_ABILITY_OPE_ADD_VLANTAG:
-		ret = get_vlan_int_val(&ability->data.vlantag.pcp,
+		ret = get_int_in_range(&ability->data.vlantag.pcp,
 				arg_val, 0, SPP_VLAN_PCP_MAX);
 		if (unlikely(ret < SPP_RET_OK)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC,
@@ -651,7 +651,7 @@ decode_classifier_vid_value(void *output, const char *arg_val,
 				int allow_override __attribute__ ((unused)))
 {
 	int ret = SPP_RET_NG;
-	ret = get_vlan_int_val(output, arg_val, 0, ETH_VLAN_ID_MAX);
+	ret = get_int_in_range(output, arg_val, 0, ETH_VLAN_ID_MAX);
 	if (unlikely(ret < SPP_RET_OK)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad VLAN ID. val=%s\n",
 				arg_val);
-- 
2.17.1

[spp] [PATCH 6/6] shared/sec: rename func for parsing lcore ID

[ogawa.yasufumi]

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

This update is to rename `decode_core_value()` to `parse_lcore_id()`
which is not for decoding but parsing.

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

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index e0da244..84e5b55 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -278,18 +278,17 @@ parse_port_uid(void *output, const char *arg_val)
 	return SPP_RET_OK;
 }
 
-/* decoding procedure of core */
+/* Parse given lcore ID. */
 static int
-decode_core_value(void *output, const char *arg_val)
+parse_lcore_id(void *output, const char *arg_val)
 {
-	int ret = SPP_RET_OK;
+	int ret;
 	ret = get_uint_in_range(output, arg_val, 0, RTE_MAX_LCORE-1);
 	if (unlikely(ret < 0)) {
-		RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad core id. val=%s\n",
-				arg_val);
+		RTE_LOG(ERR, SPP_COMMAND_PROC,
+				"Invalid lcore id '%s'.\n", arg_val);
 		return SPP_RET_NG;
 	}
-
 	return SPP_RET_OK;
 }
 
@@ -356,7 +355,7 @@ decode_component_core_value(void *output, const char *arg_val,
 	if (component->wk_action != SPPWK_ACT_START)
 		return SPP_RET_OK;
 
-	return decode_core_value(&component->core, arg_val);
+	return parse_lcore_id(&component->core, arg_val);
 }
 
 /* decoding procedure of type for component command */
-- 
2.17.1