From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A7F28A0547; Tue, 9 Feb 2021 12:20:01 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0FAEF1606DA; Tue, 9 Feb 2021 12:20:00 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id 7733940147; Tue, 9 Feb 2021 12:19:57 +0100 (CET) IronPort-SDR: rtwjOOOVQhxIrrCSZ6r3UKnwaPrj2JiQceOKjxSvRKCnYjchy7nRc/0oq9Wfa7SzZnhK4Vjudo V0ITIpnNtYXQ== X-IronPort-AV: E=McAfee;i="6000,8403,9889"; a="181926417" X-IronPort-AV: E=Sophos;i="5.81,164,1610438400"; d="scan'208";a="181926417" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Feb 2021 03:19:55 -0800 IronPort-SDR: KRFwUKHN7zryRdnlHTMeT4oyKjo6BU/h2nhpaQtFqHidfJzAk0w2qwFhkxVJik5JObewrfgn9F zTVlFmaQcXWA== X-IronPort-AV: E=Sophos;i="5.81,164,1610438400"; d="scan'208";a="378870720" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.230.90]) ([10.213.230.90]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Feb 2021 03:19:53 -0800 To: "Li, Xiaoyun" , "Singh, Jasvinder" , "Wu, Jingjing" , Adrien Mazarguil , "Dumitrescu, Cristian" Cc: "dev@dpdk.org" , "stable@dpdk.org" References: <20210205133926.779938-1-ferruh.yigit@intel.com> <20210208151520.1048763-1-ferruh.yigit@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <9ec0deaa-9cb7-c8e5-4671-bbeaa0a0a59e@intel.com> Date: Tue, 9 Feb 2021 11:19:50 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix meter commands help strings X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2/9/2021 2:35 AM, Li, Xiaoyun wrote: > Hi > >> -----Original Message----- >> From: Yigit, Ferruh >> Sent: Monday, February 8, 2021 23:15 >> To: Li, Xiaoyun ; Singh, Jasvinder >> ; Wu, Jingjing ; Adrien >> Mazarguil ; Dumitrescu, Cristian >> >> Cc: Yigit, Ferruh ; dev@dpdk.org; stable@dpdk.org >> Subject: [PATCH v2] app/testpmd: fix meter commands help strings >> >> Helps strings syntax is "command : description", the 'command' part was missing, >> updated command help strings. >> >> Fixes: 281eeb8afc55 ("app/testpmd: add commands for metering and policing") >> Fixes: 30ffb4e67ee3 ("app/testpmd: add commands traffic metering and >> policing") >> Fixes: e63b50162aa3 ("app/testpmd: clean metering and policing commands") >> Cc: stable@dpdk.org >> >> Signed-off-by: Ferruh Yigit >> --- >> Cc: jasvinder.singh@intel.com >> Cc: cristian.dumitrescu@intel.com >> >> - "set port meter dscp table" documented with 'port_id' & 'mtr_id', but >> command itself is not requiring it, can be better to double check the >> intention in the command. >> - In command "show port meter stats yes|no", it is >> not clear what 'yes|no' is, can be better to have a 'clear' keyword >> there: "show port meter stats clear yes|no" >> - 'meter' commands seems using many high level commands, that is harder >> to remember when you take all commands into account: >> "show port meter ..." >> "add port meter ..." >> "del port meter ..." >> "create port meter ..." >> "enable port meter ..." >> "disable port meter ..." >> "set port meter ..." >> And some high level commands created just for 'meter'. Instead I think >> it is better to group the commands, like: >> "port meter [add,del,create,enable,disable] ..." >> "show port meter ..." >> It is already too late but it worth to keep in mind for the possible >> future update. >> >> v2: >> * Fixed typo, actiono -> action0 >> * Added more info to help string, like "(possible values)" >> --- >> app/test-pmd/cmdline.c | 2 +- >> app/test-pmd/cmdline_mtr.c | 33 +++++++++++++++++++-------------- >> 2 files changed, 20 insertions(+), 15 deletions(-) >> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index >> 59722d268bc3..1b9da38fb745 100644 >> --- a/app/test-pmd/cmdline.c >> +++ b/app/test-pmd/cmdline.c >> @@ -719,7 +719,7 @@ static void cmd_help_long_parsed(void *parsed_result, >> "set port meter profile (port_id) (mtr_id) (profile_id)\n" >> " meter update meter profile\n\n" >> >> -"set port meter dscp table (port_id) (mtr_id) >> [(dscp_tbl_entry0)\n" >> +"set port meter dscp table [(dscp_tbl_entry0)\n" >> "(dscp_tbl_entry1)...(dscp_tbl_entry63)]\n" >> " update meter dscp table entries\n\n" >> > > I still have some concern on this one. > I understand the command itself doesn't include port_id & meter_id. > But this command use (void *)&cmd_set_port_meter_dscp_table_token_string, to take a string from cmd. > And in parser cmd_set_port_meter_dscp_table_parsed(), it will use parse_multi_token_string() to parse this token_string. > static int > parse_multi_token_string(char *t_str, uint16_t *port_id, > uint32_t *mtr_id, enum rte_color **dscp_table) > { > char *token; > uint64_t val; > int ret; > > /* First token: port id */ > token = strtok_r(t_str, PARSE_DELIMITER, &t_str); > if (token == NULL) > return -1; > > ret = parse_uint(&val, token); > if (ret != 0 || val > UINT16_MAX) > return -1; > > *port_id = val; > > /* Second token: meter id */ > token = strtok_r(t_str, PARSE_DELIMITER, &t_str); > if (token == NULL) > return 0; > > ret = parse_uint(&val, token); > if (ret != 0 || val > UINT32_MAX) > return -1; > > *mtr_id = val; > > ret = parse_dscp_table_entries(t_str, dscp_table); > if (ret != 0) > return -1; > > return 0; > } > > In this function, the first thing in that string will be parsed as port id, the second thing will be parsed as meter id and then table entries. > So I think if the helper string is "set port meter dscp table [(dscp_tbl_entry0)...]". This will actually confuse users. > If a user follows the helper and sets command "set port meter dscp table dscp_tbl_entry0", user will get error because the " dscp_tbl_entry0" is treated as portid and there is no meter id provided. > You are right, first two token of the 'token_string' is parsed as 'port_id' and 'meter_id', so intention is to have them before 'dscp_table_entries'. I will update as you suggested. > I understand they shouldn't put port id and meter id in token string parse and should be put in command itself. But that's another story. > Cristian, Jasvider, Can you please fix the command so that it gets 'port_id' and 'meter_id' as separate tokens, instead of it is being part of 'dscp_table_entries'?