Hi
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yi...@intel.com>
> Sent: Friday, February 5, 2021 21:39
> To: Lu, Wenzhuo <wenzhuo...@intel.com>; Li, Xiaoyun <xiaoyun...@intel.com>;
> Iremonger, Bernard <bernard.iremon...@intel.com>; Singh, Jasvinder
> <jasvinder.si...@intel.com>; Wu, Jingjing <jingjing...@intel.com>; Dumitrescu,
> Cristian <cristian.dumitre...@intel.com>; Adrien Mazarguil
> <adrien.mazarg...@6wind.com>
> Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; dev@dpdk.org; sta...@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: sta...@dpdk.org
>
> Signed-off-by: Ferruh Yigit <ferruh.yi...@intel.com>
> ---
> Cc: jasvinder.si...@intel.com
> Cc: cristian.dumitre...@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?
> + "<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.
> .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>]?
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
