DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Li, Xiaoyun" <xiaoyun.li@intel.com>,
	"Singh, Jasvinder" <jasvinder.singh@intel.com>,
	"Wu, Jingjing" <jingjing.wu@intel.com>,
	Adrien Mazarguil <adrien.mazarguil@6wind.com>,
	"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix meter commands help strings
Date: Tue, 9 Feb 2021 11:19:50 +0000	[thread overview]
Message-ID: <9ec0deaa-9cb7-c8e5-4671-bbeaa0a0a59e@intel.com> (raw)
In-Reply-To: <CY4PR11MB1750B5972FDE7C07C40A072B998E9@CY4PR11MB1750.namprd11.prod.outlook.com>

On 2/9/2021 2:35 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Monday, February 8, 2021 23:15
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Singh, Jasvinder
>> <jasvinder.singh@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Adrien
>> Mazarguil <adrien.mazarguil@6wind.com>; Dumitrescu, Cristian
>> <cristian.dumitrescu@intel.com>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org
>> 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: 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.
>>
>> 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.
> 

You are right, first two token of the 'token_string' is parsed as 'port_id' and 
'meter_id', so intention is to have them before 'dscp_table_entries'. I will 
update as you suggested.

> 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.
> 

Cristian, Jasvider,

Can you please fix the command so that it gets 'port_id' and 'meter_id' as 
separate tokens, instead of it is being part of 'dscp_table_entries'?

  reply	other threads:[~2021-02-09 11:20 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-05 13:39 [dpdk-dev] [PATCH] " Ferruh Yigit
2021-02-07  2:47 ` Li, Xiaoyun
2021-02-08 14:40   ` Ferruh Yigit
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 [this message]
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=9ec0deaa-9cb7-c8e5-4671-bbeaa0a0a59e@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=adrien.mazarguil@6wind.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=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).