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 179CEA0547; Mon, 8 Feb 2021 15:40:57 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8288E4014E; Mon, 8 Feb 2021 15:40:56 +0100 (CET) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id 507EA40147; Mon, 8 Feb 2021 15:40:54 +0100 (CET) IronPort-SDR: fji/idg1q2YemFVjFYehX+57cJ2bfaQwb80LsdPnyQhG3kxLbzs4hs/M30wrGqEUENl0DLfndC g6cLdhKikBmQ== X-IronPort-AV: E=McAfee;i="6000,8403,9888"; a="245791229" X-IronPort-AV: E=Sophos;i="5.81,162,1610438400"; d="scan'208";a="245791229" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Feb 2021 06:40:51 -0800 IronPort-SDR: RAd+ca6FrBmy1zNYbgdPUfN2ip7mpbXScAW4FIczuCKTieFfvAlEdkHKpZ4EX0D5ErvXu7/haY tcYclj3F8VJQ== X-IronPort-AV: E=Sophos;i="5.81,162,1610438400"; d="scan'208";a="377796800" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.15.217]) ([10.252.15.217]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Feb 2021 06:40:48 -0800 To: "Li, Xiaoyun" , "Lu, Wenzhuo" , "Iremonger, Bernard" , "Singh, Jasvinder" , "Wu, Jingjing" , "Dumitrescu, Cristian" , Adrien Mazarguil Cc: "dev@dpdk.org" , "stable@dpdk.org" References: <20210205133926.779938-1-ferruh.yigit@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <94ec2310-1c40-259e-b9db-f088b812678f@ferruh> Date: Mon, 8 Feb 2021 14:40:43 +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] 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/7/2021 2:47 AM, Li, Xiaoyun wrote: > Hi > >> -----Original Message----- >> From: Yigit, Ferruh >> Sent: Friday, February 5, 2021 21:39 >> To: Lu, Wenzhuo ; Li, Xiaoyun ; >> Iremonger, Bernard ; Singh, Jasvinder >> ; Wu, Jingjing ; Dumitrescu, >> Cristian ; Adrien Mazarguil >> >> Cc: Yigit, Ferruh ; dev@dpdk.org; stable@dpdk.org >> Subject: [PATCH] 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. >> --- >> app/test-pmd/cmdline.c | 2 +- >> app/test-pmd/cmdline_mtr.c | 32 ++++++++++++++++++-------------- >> 2 files changed, 19 insertions(+), 15 deletions(-) > >> @@ -827,7 +827,9 @@ static void cmd_create_port_meter_parsed(void >> *parsed_result, cmdline_parse_inst_t cmd_create_port_meter = { >> .f = cmd_create_port_meter_parsed, >> .data = NULL, >> -.help_str = "Create port meter", >> +.help_str = "create port meter " >> +"yes|no R|Y|G|D R|Y|G|D R|Y|G|D " > > It seems it should be R|Y|G|D|r|y|g|d R|Y|G|D|r|y|g|d R|Y|G|D|r|y|g|d. > What about just use as in cmd_help_long_parsed? > Yes action can be both upper case and lower case, but both does same thing, so I chose only uppercase ones to make it more readable. The "" makes it easy understand what the field is for, but for the user that doesn't know what is "g_action" or what can be the values "g_action" can take, there is no place to get this information. That is why I find it useful to provide values can take if they are fixed. >> +" [ >> +...]", >> .tokens = { >> (void *)&cmd_create_port_meter_create, >> (void *)&cmd_create_port_meter_port, > >> cmd_set_port_meter_dscp_table_parsed(void *parsed_result, >> cmdline_parse_inst_t cmd_set_port_meter_dscp_table = { >> .f = cmd_set_port_meter_dscp_table_parsed, >> .data = NULL, >> -.help_str = "Update port meter dscp table", >> +.help_str = "set port meter dscp table " >> +"[ ... ]", > > It should be "set port meter dscp table " > "[ ... ]"? > Because in parse_multi_token_string(), it starts from port_id. Hmmm... The code seems very messy, not inconsistent. > Please see my comment above, copied here: - "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. If you check the code, port_id and mtr_id is not there. >> .tokens = { >> (void *)&cmd_set_port_meter_dscp_table_set, >> (void *)&cmd_set_port_meter_dscp_table_port, >> @@ -1276,7 +1279,8 @@ static void >> cmd_set_port_meter_policer_action_parsed(void *parsed_result, >> cmdline_parse_inst_t cmd_set_port_meter_policer_action = { >> .f = cmd_set_port_meter_policer_action_parsed, >> .data = NULL, >> -.help_str = "Set port meter policer action", >> +.help_str = "set port meter policer action " >> +" [ ]", > > [ ]? ack > Since it seems only 3 actions exist (action_mask & (~0x7UL), check action mask part in parse function). > And each action seems to be G|Y|R|D|g|y|r|d. hmmm. Messy code... > >> .tokens = { >> (void *)&cmd_set_port_meter_policer_action_set, >> (void *)&cmd_set_port_meter_policer_action_port, > >> -- >> 2.29.2 >