[spp] [PATCH 0/4] Revise name of funcs and data for parsing

[spp] [PATCH 0/4] Revise name of funcs and data for parsing

[ogawa.yasufumi]

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

This series of patches is to revise name of functions and data
structures because they are incorrect or ambiguous in meaning.

Yasufumi Ogawa (4):
  shared/sec: rename define for empty params
  shared/sec: rename struct of cmd operators
  shared/sec: rename func for parsing given command
  shared/sec: rename operator func for parsing cmds

 .../secondary/spp_worker_th/cmd_parser.c      | 189 ++++++++++--------
 .../secondary/spp_worker_th/cmd_parser.h      |   2 +-
 2 files changed, 106 insertions(+), 85 deletions(-)

-- 
2.17.1

[spp] [PATCH 1/4] shared/sec: rename define for empty params

[ogawa.yasufumi]

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

The name of define `DECODE_PARAMETER_LIST_EMPTY` is a set of null vars
used for command which takes no params. It is redundant and ambiguous
for meaning. This update is to rename to `SPPWK_CMD_NO_PARAMS`.

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

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index 84e5b55..6122cee 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -710,8 +710,6 @@ parse_cls_port(void *cls_cmd_attr, const char *arg_val,
 	return SPP_RET_OK;
 }
 
-#define DECODE_PARAMETER_LIST_EMPTY { NULL, 0, NULL }
-
 /* parameter list for decoding */
 struct decode_parameter_list {
 	const char *name;       /* Parameter name */
@@ -720,10 +718,13 @@ struct decode_parameter_list {
 				/* Pointer to parameter handling function */
 };
 
+/* Used for command which takes no params, such as `status`. */
+#define SPPWK_CMD_NO_PARAMS { NULL, 0, NULL }
+
 /* parameter list for each command */
 static struct decode_parameter_list
 parameter_list[][SPPWK_MAX_PARAMS] = {
-	{                                /* classifier_table(mac) */
+	{  /* classifier_table(mac) */
 		{
 			.name = "action",
 			.offset = offsetof(struct spp_command,
@@ -748,9 +749,9 @@ parameter_list[][SPPWK_MAX_PARAMS] = {
 					spec.cls_table),
 			.func = parse_cls_port
 		},
-		DECODE_PARAMETER_LIST_EMPTY,
+		SPPWK_CMD_NO_PARAMS,
 	},
-	{                                /* classifier_table(VLAN) */
+	{  /* classifier_table(VLAN) */
 		{
 			.name = "action",
 			.offset = offsetof(struct spp_command,
@@ -781,12 +782,12 @@ parameter_list[][SPPWK_MAX_PARAMS] = {
 					spec.cls_table),
 			.func = parse_cls_port
 		},
-		DECODE_PARAMETER_LIST_EMPTY,
+		SPPWK_CMD_NO_PARAMS,
 	},
-	{ DECODE_PARAMETER_LIST_EMPTY }, /* _get_client_id   */
-	{ DECODE_PARAMETER_LIST_EMPTY }, /* status           */
-	{ DECODE_PARAMETER_LIST_EMPTY }, /* exit             */
-	{                                /* component        */
+	{ SPPWK_CMD_NO_PARAMS },  /* _get_client_id */
+	{ SPPWK_CMD_NO_PARAMS },  /* status */
+	{ SPPWK_CMD_NO_PARAMS },  /* exit */
+	{  /* component */
 		{
 			.name = "action",
 			.offset = offsetof(struct spp_command,
@@ -808,9 +809,9 @@ parameter_list[][SPPWK_MAX_PARAMS] = {
 			.offset = offsetof(struct spp_command, spec.comp),
 			.func = decode_component_type_value
 		},
-		DECODE_PARAMETER_LIST_EMPTY,
+		SPPWK_CMD_NO_PARAMS,
 	},
-	{                                /* port             */
+	{  /* port */
 		{
 			.name = "action",
 			.offset = offsetof(struct spp_command,
@@ -847,9 +848,9 @@ parameter_list[][SPPWK_MAX_PARAMS] = {
 			.offset = offsetof(struct spp_command, spec.port),
 			.func = decode_port_pcp
 		},
-		DECODE_PARAMETER_LIST_EMPTY,
+		SPPWK_CMD_NO_PARAMS,
 	},
-	{ DECODE_PARAMETER_LIST_EMPTY }, /* termination      */
+	{ SPPWK_CMD_NO_PARAMS }, /* termination */
 };
 
 /* Validate given command. */
-- 
2.17.1

[spp] [PATCH 2/4] shared/sec: rename struct of cmd operators

[ogawa.yasufumi]

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

A set of operators and its names for parsing command is defined as
`decode_parameter_list`, but the name is inappropriate considering
the purpose. This update is to rename it to `sppwk_cmd_ops` instead.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 .../secondary/spp_worker_th/cmd_parser.c      | 36 +++++++++----------
 .../secondary/spp_worker_th/cmd_parser.h      |  2 +-
 2 files changed, 19 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 6122cee..853b0ab 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -659,7 +659,7 @@ decode_classifier_vid_value(void *output, const char *arg_val,
 	return SPP_RET_OK;
 }
 
-/* decoding procedure of port for classifier_table command */
+/* Parse port for classifier_table command */
 static int
 parse_cls_port(void *cls_cmd_attr, const char *arg_val,
 		int allow_override __attribute__ ((unused)))
@@ -710,20 +710,20 @@ parse_cls_port(void *cls_cmd_attr, const char *arg_val,
 	return SPP_RET_OK;
 }
 
-/* parameter list for decoding */
-struct decode_parameter_list {
-	const char *name;       /* Parameter name */
-	size_t offset;          /* Offset value of struct spp_command */
+/* Attributes operator functions of command for parsing. */
+struct sppwk_cmd_ops {
+	const char *name;
+	size_t offset;  /* Offset of struct spp_command */
+	/* Pointer to operator function */
 	int (*func)(void *output, const char *arg_val, int allow_override);
-				/* Pointer to parameter handling function */
 };
 
 /* Used for command which takes no params, such as `status`. */
 #define SPPWK_CMD_NO_PARAMS { NULL, 0, NULL }
 
-/* parameter list for each command */
-static struct decode_parameter_list
-parameter_list[][SPPWK_MAX_PARAMS] = {
+/* A set of operator functions for parsing command. */
+static struct sppwk_cmd_ops
+cmd_ops_list[][SPPWK_MAX_PARAMS] = {
 	{  /* classifier_table(mac) */
 		{
 			.name = "action",
@@ -863,11 +863,11 @@ decode_command_parameter_component(struct sppwk_cmd_req *request,
 	int ret = SPP_RET_OK;
 	int ci = request->commands[0].type;
 	int pi = 0;
-	struct decode_parameter_list *list = NULL;
+	struct sppwk_cmd_ops *list = NULL;
 	for (pi = 1; pi < argc; pi++) {
-		list = &parameter_list[ci][pi-1];
+		list = &cmd_ops_list[ci][pi-1];
 		ret = (*list->func)((void *)
-				((char *)&request->commands[0]+list->offset),
+				((char *)&request->commands[0] + list->offset),
 				argv[pi], 0);
 		if (unlikely(ret < 0)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC,
@@ -904,11 +904,11 @@ decode_command_parameter_cls_table_vlan(struct sppwk_cmd_req *request,
 	int ret = SPP_RET_OK;
 	int ci = request->commands[0].type;
 	int pi = 0;
-	struct decode_parameter_list *list = NULL;
+	struct sppwk_cmd_ops *list = NULL;
 	for (pi = 1; pi < argc; pi++) {
-		list = &parameter_list[ci][pi-1];
+		list = &cmd_ops_list[ci][pi-1];
 		ret = (*list->func)((void *)
-				((char *)&request->commands[0]+list->offset),
+				((char *)&request->commands[0] + list->offset),
 				argv[pi], 0);
 		if (unlikely(ret < SPP_RET_OK)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad value. "
@@ -931,7 +931,7 @@ decode_command_parameter_port(struct sppwk_cmd_req *request,
 	int ret = SPP_RET_OK;
 	int ci = request->commands[0].type;
 	int pi = 0;
-	struct decode_parameter_list *list = NULL;
+	struct sppwk_cmd_ops *list = NULL;
 	int flag = 0;
 
 	/* check add vlatag */
@@ -939,9 +939,9 @@ decode_command_parameter_port(struct sppwk_cmd_req *request,
 		flag = 1;
 
 	for (pi = 1; pi < argc; pi++) {
-		list = &parameter_list[ci][pi-1];
+		list = &cmd_ops_list[ci][pi-1];
 		ret = (*list->func)((void *)
-				((char *)&request->commands[0]+list->offset),
+				((char *)&request->commands[0] + list->offset),
 				argv[pi], flag);
 		if (unlikely(ret < SPP_RET_OK)) {
 			RTE_LOG(ERR, SPP_COMMAND_PROC, "Bad value. "
diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.h b/src/shared/secondary/spp_worker_th/cmd_parser.h
index 58e39a9..286fde0 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.h
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.h
@@ -112,7 +112,7 @@ struct spp_command {
 		struct sppwk_cmd_flush flush;
 		struct sppwk_cmd_comp comp;
 		struct sppwk_cmd_port port;
-	} spec;
+	} spec;  /* TODO(yasufum) rename no reasonable name */
 };
 
 /* Request parameters. */
-- 
2.17.1

[spp] [PATCH 3/4] shared/sec: rename func for parsing given command

[ogawa.yasufumi]

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

The name `decode_command_in_list()` is redundant and inappropriate
because it is for parsing a string of command and not for `in list`,
and not for decoding.

This update is to rename this func to `parse_wk_cmd()` simply. Although
this function has problems, such as unclear naming of vars and usages,
this update does not fix them, but add TODO comments for fixing later.

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

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index 853b0ab..b553ae0 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -186,6 +186,11 @@ set_detailed_parse_error(struct sppwk_parse_err_msg *wk_err_msg,
 }
 
 /* Split command line paramis with spaces. */
+/**
+ * TODO(yasufum) It should be renamed because this function checks if the num
+ * of params is over given max num, but this behaviour is not explicit in the
+ * name of function. Or remove this checking for simplicity.
+ */
 static int
 split_cmd_params(char *string, int max, int *argc, char *argv[])
 {
@@ -954,19 +959,24 @@ decode_command_parameter_port(struct sppwk_cmd_req *request,
 	return SPP_RET_OK;
 }
 
-/* command list for decoding */
-struct decode_command_list {
-	const char *name;       /* Command name */
-	int   param_min;        /* Min number of parameters */
-	int   param_max;        /* Max number of parameters */
+/**
+ * Attributes of commands for parsing. The last member of function pointer
+ * is the operator function for the command.
+ */
+struct cmd_parse_attrs {
+	const char *cmd_name;
+	int nof_params_min;
+	int nof_params_max;
 	int (*func)(struct sppwk_cmd_req *request, int argc,
 			char *argv[], struct sppwk_parse_err_msg *wk_err_msg,
 			int maxargc);
-				/* Pointer to command handling function */
 };
 
-/* command list */
-static struct decode_command_list command_list[] = {
+/**
+ * List of command attributes defines the name of command, number of params
+ * and operator functions.
+ */
+static struct cmd_parse_attrs cmd_attr_list[] = {
 	{ "classifier_table", 5, 5, decode_command_parameter_cls_table },
 	{ "classifier_table", 6, 6, decode_command_parameter_cls_table_vlan },
 	{ "_get_client_id", 1, 1, NULL },
@@ -977,16 +987,21 @@ static struct decode_command_list command_list[] = {
 	{ "", 0, 0, NULL }  /* termination */
 };
 
-/* Parse command line parameters. */
+/* Parse command for SPP worker. */
 static int
-decode_command_in_list(struct sppwk_cmd_req *request,
-			const char *request_str,
-			struct sppwk_parse_err_msg *wk_err_msg)
+parse_wk_cmd(struct sppwk_cmd_req *request,
+		const char *request_str,
+		struct sppwk_parse_err_msg *wk_err_msg)
 {
 	int ret = SPP_RET_OK;
-	int command_name_check = 0;
-	struct decode_command_list *list = NULL;
+	int is_valid_nof_params = 1;  /* for checking nof params in range. */
+	struct cmd_parse_attrs *list = NULL;
 	int i = 0;
+	/**
+	 * TODO(yasufum) The name of `argc` and `argv` should be renamed because
+	 * it is used for the num of params and param itself, not for arguments.
+	 * It is so misunderstandable for maintainance.
+	 */
 	int argc = 0;
 	char *argv[SPPWK_MAX_PARAMS];
 	char tmp_str[SPPWK_MAX_PARAMS*SPPWK_VAL_BUFSZ];
@@ -994,43 +1009,57 @@ decode_command_in_list(struct sppwk_cmd_req *request,
 	memset(tmp_str, 0x00, sizeof(tmp_str));
 
 	strcpy(tmp_str, request_str);
+	/**
+	 * TODO(yasufum) As described in the definition of
+	 * `split_cmd_params()`, the name and usage of this function should
+	 * be refactored because it is no meaning to check the num of params
+	 * here. The checking is not explicit in the name of func, and checking
+	 * itself is done in the next step as following. No need to do here.
+	 */
 	ret = split_cmd_params(tmp_str, SPPWK_MAX_PARAMS, &argc, argv);
 	if (ret < SPP_RET_OK) {
-		RTE_LOG(ERR, SPP_COMMAND_PROC, "Parameter number over limit."
-				"request_str=%s\n", request_str);
+		RTE_LOG(ERR, SPP_COMMAND_PROC,
+				"Num of params should be less than %d. "
+				"request_str=%s\n",
+				SPPWK_MAX_PARAMS, request_str);
 		return set_parse_error(wk_err_msg, SPPWK_PARSE_WRONG_FORMAT,
 				NULL);
 	}
 	RTE_LOG(DEBUG, SPP_COMMAND_PROC, "Decode array. num=%d\n", argc);
 
-	for (i = 0; command_list[i].name[0] != '\0'; i++) {
-		list = &command_list[i];
-		if (strcmp(argv[0], list->name) != 0)
+	for (i = 0; cmd_attr_list[i].cmd_name[0] != '\0'; i++) {
+		list = &cmd_attr_list[i];
+		if (strcmp(argv[0], list->cmd_name) != 0)
 			continue;
 
-		if (unlikely(argc < list->param_min) ||
-				unlikely(list->param_max < argc)) {
-			command_name_check = 1;
+		if (unlikely(argc < list->nof_params_min) ||
+				unlikely(list->nof_params_max < argc)) {
+			is_valid_nof_params = 0;
 			continue;
 		}
 
 		request->commands[0].type = i;
 		if (list->func != NULL)
 			return (*list->func)(request, argc, argv, wk_err_msg,
-							list->param_max);
+							list->nof_params_max);
 
 		return SPP_RET_OK;
 	}
 
-	if (command_name_check != 0) {
-		RTE_LOG(ERR, SPP_COMMAND_PROC, "Parameter number out of range."
+	/**
+	 * Failed to parse command because of invalid nof params or
+	 * unknown command.
+	 */
+	if (is_valid_nof_params == 0) {
+		RTE_LOG(ERR, SPP_COMMAND_PROC,
+				"Number of parmas is out of range. "
 				"request_str=%s\n", request_str);
 		return set_parse_error(wk_err_msg, SPPWK_PARSE_WRONG_FORMAT,
 				NULL);
 	}
 
 	RTE_LOG(ERR, SPP_COMMAND_PROC,
-			"Unknown command. command=%s, request_str=%s\n",
+			"Unknown command '%s' and request_str=%s\n",
 			argv[0], request_str);
 	return set_detailed_parse_error(wk_err_msg, "command", argv[0]);
 }
@@ -1047,7 +1076,7 @@ sppwk_parse_req(
 
 	/* decode request */
 	request->num_command = 1;
-	ret = decode_command_in_list(request, request_str, wk_err_msg);
+	ret = parse_wk_cmd(request, request_str, wk_err_msg);
 	if (unlikely(ret != SPP_RET_OK)) {
 		RTE_LOG(ERR, SPP_COMMAND_PROC,
 				"Cannot decode command request. "
-- 
2.17.1

[spp] [PATCH 4/4] shared/sec: rename operator func for parsing cmds

[ogawa.yasufumi]

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

Operator functions for parsing command is prefixed as
`decode_command_parameter_` are renamed to `parse_cmd_` because for
too long and not intuitive.

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

diff --git a/src/shared/secondary/spp_worker_th/cmd_parser.c b/src/shared/secondary/spp_worker_th/cmd_parser.c
index b553ae0..ae845f4 100644
--- a/src/shared/secondary/spp_worker_th/cmd_parser.c
+++ b/src/shared/secondary/spp_worker_th/cmd_parser.c
@@ -860,10 +860,9 @@ cmd_ops_list[][SPPWK_MAX_PARAMS] = {
 
 /* Validate given command. */
 static int
-decode_command_parameter_component(struct sppwk_cmd_req *request,
-				int argc, char *argv[],
-				struct sppwk_parse_err_msg *wk_err_msg,
-				int maxargc __attribute__ ((unused)))
+parse_cmd_comp(struct sppwk_cmd_req *request, int argc, char *argv[],
+		struct sppwk_parse_err_msg *wk_err_msg,
+		int maxargc __attribute__ ((unused)))
 {
 	int ret = SPP_RET_OK;
 	int ci = request->commands[0].type;
@@ -888,23 +887,17 @@ decode_command_parameter_component(struct sppwk_cmd_req *request,
 
 /* Validate given command for clssfier_table. */
 static int
-decode_command_parameter_cls_table(struct sppwk_cmd_req *request,
-				int argc, char *argv[],
-				struct sppwk_parse_err_msg *wk_err_msg,
-				int maxargc)
+parse_cmd_cls_table(struct sppwk_cmd_req *request, int argc, char *argv[],
+		struct sppwk_parse_err_msg *wk_err_msg, int maxargc)
 {
-	return decode_command_parameter_component(request,
-						argc,
-						argv,
-						wk_err_msg,
-						maxargc);
+	return parse_cmd_comp(request, argc, argv, wk_err_msg, maxargc);
 }
+
 /* Validate given command for clssfier_table of vlan. */
 static int
-decode_command_parameter_cls_table_vlan(struct sppwk_cmd_req *request,
-				int argc, char *argv[],
-				struct sppwk_parse_err_msg *wk_err_msg,
-				int maxargc __attribute__ ((unused)))
+parse_cmd_cls_table_vlan(struct sppwk_cmd_req *request, int argc, char *argv[],
+		struct sppwk_parse_err_msg *wk_err_msg,
+		int maxargc __attribute__ ((unused)))
 {
 	int ret = SPP_RET_OK;
 	int ci = request->commands[0].type;
@@ -928,10 +921,8 @@ decode_command_parameter_cls_table_vlan(struct sppwk_cmd_req *request,
 
 /* Validate given command for port. */
 static int
-decode_command_parameter_port(struct sppwk_cmd_req *request,
-				int argc, char *argv[],
-				struct sppwk_parse_err_msg *wk_err_msg,
-				int maxargc)
+parse_cmd_port(struct sppwk_cmd_req *request, int argc, char *argv[],
+		struct sppwk_parse_err_msg *wk_err_msg, int maxargc)
 {
 	int ret = SPP_RET_OK;
 	int ci = request->commands[0].type;
@@ -977,13 +968,13 @@ struct cmd_parse_attrs {
  * and operator functions.
  */
 static struct cmd_parse_attrs cmd_attr_list[] = {
-	{ "classifier_table", 5, 5, decode_command_parameter_cls_table },
-	{ "classifier_table", 6, 6, decode_command_parameter_cls_table_vlan },
+	{ "classifier_table", 5, 5, parse_cmd_cls_table },
+	{ "classifier_table", 6, 6, parse_cmd_cls_table_vlan },
 	{ "_get_client_id", 1, 1, NULL },
 	{ "status", 1, 1, NULL },
 	{ "exit", 1, 1, NULL },
-	{ "component", 3, 5, decode_command_parameter_component },
-	{ "port", 5, 8, decode_command_parameter_port },
+	{ "component", 3, 5, parse_cmd_comp },
+	{ "port", 5, 8, parse_cmd_port },
 	{ "", 0, 0, NULL }  /* termination */
 };
 
-- 
2.17.1