DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Li, Xiaoyun" <xiaoyun.li@intel.com>,
	"Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
	"Iremonger, Bernard" <bernard.iremonger@intel.com>,
	"Singh, Jasvinder" <jasvinder.singh@intel.com>,
	"Wu, Jingjing" <jingjing.wu@intel.com>,
	"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>,
	Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix meter commands help strings
Date: Mon, 8 Feb 2021 14:40:43 +0000	[thread overview]
Message-ID: <94ec2310-1c40-259e-b9db-f088b812678f@ferruh> (raw)
In-Reply-To: <CY4PR11MB1750AD6B2C1157B61756C2B399B09@CY4PR11MB1750.namprd11.prod.outlook.com>

On 2/7/2021 2:47 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Friday, February 5, 2021 21:39
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>> Iremonger, Bernard <bernard.iremonger@intel.com>; Singh, Jasvinder
>> <jasvinder.singh@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Dumitrescu,
>> Cristian <cristian.dumitrescu@intel.com>; Adrien Mazarguil
>> <adrien.mazarguil@6wind.com>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; 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 <ferruh.yigit@intel.com>
>> ---
>> 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 <port_id> <mtr_id> yes|no", it is
>>    not clear what 'yes|no' is, can be better to have a 'clear' keyword
>>    there: "show port meter stats <port_id> <mtr_id> 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(-)
> <snip>
>> @@ -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 <port_id> <mtr_id> <profile_id> "
>> +"yes|no R|Y|G|D R|Y|G|D R|Y|G|D <stats_mask> <shared> "
> 
> 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 <g_action> <y_action> <r_action> 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 "<g_action>" 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.

>> +"<use_pre_meter_color> [<dscp_tbl_entry0> <dscp_tbl_entry1>
>> +...<dscp_tbl_entry63>]",
>>   .tokens = {
>>   (void *)&cmd_create_port_meter_create,
>>   (void *)&cmd_create_port_meter_port,
> <snip>
>> 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 "
>> +"[<dscp_tbl_entry0> <dscp_tbl_entry1> ... <dscp_tbl_entry63>]",
> 
> It should be "set port meter dscp table <port_id> <mtr_id> "
>                        "[<dscp_tbl_entry0> <dscp_tbl_entry1> ... <dscp_tbl_entry63>]"?
> 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 <port_id> <mtr_id> "
>> +"<action_mask> <actiono> [<action1> <action2>]",
> 
> <action0>[ <action1> <action2>]?

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,
> <snip>
>> --
>> 2.29.2
> 


  reply	other threads:[~2021-02-08 14:40 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-05 13:39 Ferruh Yigit
2021-02-07  2:47 ` Li, Xiaoyun
2021-02-08 14:40   ` Ferruh Yigit [this message]
2021-02-08 15:15 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2021-02-09  2:35   ` Li, Xiaoyun
2021-02-09 11:19     ` Ferruh Yigit
2021-02-09 14:15 ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
2021-02-09 15:32   ` Singh, Jasvinder
2021-02-10 20:59     ` Thomas Monjalon
2021-02-10  0:28   ` Li, Xiaoyun

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=94ec2310-1c40-259e-b9db-f088b812678f@ferruh \
    --to=ferruh.yigit@intel.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=bernard.iremonger@intel.com \
    --cc=cristian.dumitrescu@intel.com \
    --cc=dev@dpdk.org \
    --cc=jasvinder.singh@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=stable@dpdk.org \
    --cc=wenzhuo.lu@intel.com \
    --cc=xiaoyun.li@intel.com \
    /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).