[dpdk-dev] [PATCH] app/testpmd: fix meter commands help strings
Ferruh Yigit
ferruh.yigit at intel.com
Mon Feb 8 15:40:43 CET 2021
On 2/7/2021 2:47 AM, Li, Xiaoyun wrote:
> Hi
>
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit at intel.com>
>> Sent: Friday, February 5, 2021 21:39
>> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; Li, Xiaoyun <xiaoyun.li at intel.com>;
>> Iremonger, Bernard <bernard.iremonger at intel.com>; Singh, Jasvinder
>> <jasvinder.singh at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>; Dumitrescu,
>> Cristian <cristian.dumitrescu at intel.com>; Adrien Mazarguil
>> <adrien.mazarguil at 6wind.com>
>> Cc: Yigit, Ferruh <ferruh.yigit at intel.com>; dev at dpdk.org; stable at 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 at dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit at intel.com>
>> ---
>> Cc: jasvinder.singh at intel.com
>> Cc: cristian.dumitrescu at 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
>
More information about the dev
mailing list