From: ogawa.yasufumi@lab.ntt.co.jp
To: spp@dpdk.org, ferruh.yigit@intel.com, ogawa.yasufumi@lab.ntt.co.jp
Subject: [spp] [PATCH 3/4] shared/sec: rename func for parsing given command
Date: Tue, 21 May 2019 11:31:42 +0900 [thread overview]
Message-ID: <1558405903-8252-4-git-send-email-ogawa.yasufumi@lab.ntt.co.jp> (raw)
In-Reply-To: <1558405903-8252-1-git-send-email-ogawa.yasufumi@lab.ntt.co.jp>
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
next prev parent reply other threads:[~2019-05-21 2:34 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-21 2:31 [spp] [PATCH 0/4] Revise name of funcs and data for parsing ogawa.yasufumi
2019-05-21 2:31 ` [spp] [PATCH 1/4] shared/sec: rename define for empty params ogawa.yasufumi
2019-05-21 2:31 ` [spp] [PATCH 2/4] shared/sec: rename struct of cmd operators ogawa.yasufumi
2019-05-21 2:31 ` ogawa.yasufumi [this message]
2019-05-21 2:31 ` [spp] [PATCH 4/4] shared/sec: rename operator func for parsing cmds ogawa.yasufumi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1558405903-8252-4-git-send-email-ogawa.yasufumi@lab.ntt.co.jp \
--to=ogawa.yasufumi@lab.ntt.co.jp \
--cc=ferruh.yigit@intel.com \
--cc=spp@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).