From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 7673BA05D3 for ; Tue, 21 May 2019 04:34:11 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6BF13A3; Tue, 21 May 2019 04:34:11 +0200 (CEST) Received: from tama50.ecl.ntt.co.jp (tama50.ecl.ntt.co.jp [129.60.39.147]) by dpdk.org (Postfix) with ESMTP id CAA82A3 for ; Tue, 21 May 2019 04:34:09 +0200 (CEST) Received: from vc2.ecl.ntt.co.jp (vc2.ecl.ntt.co.jp [129.60.86.154]) by tama50.ecl.ntt.co.jp (8.13.8/8.13.8) with ESMTP id x4L2Y892003191; Tue, 21 May 2019 11:34:08 +0900 Received: from vc2.ecl.ntt.co.jp (localhost [127.0.0.1]) by vc2.ecl.ntt.co.jp (Postfix) with ESMTP id ADF18638A06; Tue, 21 May 2019 11:34:08 +0900 (JST) Received: from localhost.localdomain (lobster.nslab.ecl.ntt.co.jp [129.60.13.95]) by vc2.ecl.ntt.co.jp (Postfix) with ESMTP id 9FE9C6387B4; Tue, 21 May 2019 11:34:08 +0900 (JST) From: ogawa.yasufumi@lab.ntt.co.jp To: spp@dpdk.org, ferruh.yigit@intel.com, ogawa.yasufumi@lab.ntt.co.jp Date: Tue, 21 May 2019 11:31:42 +0900 Message-Id: <1558405903-8252-4-git-send-email-ogawa.yasufumi@lab.ntt.co.jp> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1558405903-8252-1-git-send-email-ogawa.yasufumi@lab.ntt.co.jp> References: <1558405903-8252-1-git-send-email-ogawa.yasufumi@lab.ntt.co.jp> X-TM-AS-MML: disable Subject: [spp] [PATCH 3/4] shared/sec: rename func for parsing given command X-BeenThere: spp@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Soft Patch Panel List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: spp-bounces@dpdk.org Sender: "spp" From: Yasufumi Ogawa 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 --- .../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