Hi
> -----Original Message-----
> From: Yigit, Ferruh <[email protected]>
> Sent: Monday, February 8, 2021 23:15
> To: Li, Xiaoyun <[email protected]>; Singh, Jasvinder
> <[email protected]>; Wu, Jingjing <[email protected]>; Adrien
> Mazarguil <[email protected]>; Dumitrescu, Cristian
> <[email protected]>
> Cc: Yigit, Ferruh <[email protected]>; [email protected]; [email protected]
> 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: [email protected]
>
> Signed-off-by: Ferruh Yigit <[email protected]>
> ---
> Cc: [email protected]
> Cc: [email protected]
>
> - "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.
>
> v2:
> * Fixed typo, actiono -> action0
> * Added more info to help string, like "<variable>(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.
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.
> --
> 2.29.2
