[PATCH iproute2] dcb: app: Add missing "dcb app show dev X default-prio"

[PATCH iproute2] dcb: app: Add missing "dcb app show dev X default-prio"

[Petr Machata]

All the actual code exists, but we neglect to recognize "default-prio" as a
CLI key for selection of what to show.

Reported-by: Maksym Yaremchuk <maksymy@nvidia.com>
Tested-by: Maksym Yaremchuk <maksymy@nvidia.com>
Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 dcb/dcb_app.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dcb/dcb_app.c b/dcb/dcb_app.c
index 28f40614..54a95a07 100644
--- a/dcb/dcb_app.c
+++ b/dcb/dcb_app.c
@@ -646,6 +646,8 @@ static int dcb_cmd_app_show(struct dcb *dcb, const char *dev, int argc, char **a
 			goto out;
 		} else if (matches(*argv, "ethtype-prio") == 0) {
 			dcb_app_print_ethtype_prio(&tab);
+		} else if (matches(*argv, "default-prio") == 0) {
+			dcb_app_print_default_prio(&tab);
 		} else if (matches(*argv, "dscp-prio") == 0) {
 			dcb_app_print_dscp_prio(dcb, &tab);
 		} else if (matches(*argv, "stream-port-prio") == 0) {
-- 
2.31.1

Re: [PATCH iproute2] dcb: app: Add missing "dcb app show dev X default-prio"

[David Ahern]

On 1/18/22 4:36 AM, Petr Machata wrote:
> All the actual code exists, but we neglect to recognize "default-prio" as a
> CLI key for selection of what to show.
> 
> Reported-by: Maksym Yaremchuk <maksymy@nvidia.com>
> Tested-by: Maksym Yaremchuk <maksymy@nvidia.com>
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
>  dcb/dcb_app.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/dcb/dcb_app.c b/dcb/dcb_app.c
> index 28f40614..54a95a07 100644
> --- a/dcb/dcb_app.c
> +++ b/dcb/dcb_app.c
> @@ -646,6 +646,8 @@ static int dcb_cmd_app_show(struct dcb *dcb, const char *dev, int argc, char **a
>  			goto out;
>  		} else if (matches(*argv, "ethtype-prio") == 0) {
>  			dcb_app_print_ethtype_prio(&tab);
> +		} else if (matches(*argv, "default-prio") == 0) {
> +			dcb_app_print_default_prio(&tab);
>  		} else if (matches(*argv, "dscp-prio") == 0) {
>  			dcb_app_print_dscp_prio(dcb, &tab);
>  		} else if (matches(*argv, "stream-port-prio") == 0) {

In general, we are not allowing more uses of matches(). I think this one
can be an exception for consistency with the other options, so really
just a heads up.

Re: [PATCH iproute2] dcb: app: Add missing "dcb app show dev X default-prio"

[Petr Machata]

David Ahern <dsahern@gmail.com> writes:

> On 1/18/22 4:36 AM, Petr Machata wrote:
>> All the actual code exists, but we neglect to recognize "default-prio" as a
>> CLI key for selection of what to show.
>> 
>> Reported-by: Maksym Yaremchuk <maksymy@nvidia.com>
>> Tested-by: Maksym Yaremchuk <maksymy@nvidia.com>
>> Signed-off-by: Petr Machata <petrm@nvidia.com>
>> ---
>>  dcb/dcb_app.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/dcb/dcb_app.c b/dcb/dcb_app.c
>> index 28f40614..54a95a07 100644
>> --- a/dcb/dcb_app.c
>> +++ b/dcb/dcb_app.c
>> @@ -646,6 +646,8 @@ static int dcb_cmd_app_show(struct dcb *dcb, const char *dev, int argc, char **a
>>  			goto out;
>>  		} else if (matches(*argv, "ethtype-prio") == 0) {
>>  			dcb_app_print_ethtype_prio(&tab);
>> +		} else if (matches(*argv, "default-prio") == 0) {
>> +			dcb_app_print_default_prio(&tab);
>>  		} else if (matches(*argv, "dscp-prio") == 0) {
>>  			dcb_app_print_dscp_prio(dcb, &tab);
>>  		} else if (matches(*argv, "stream-port-prio") == 0) {
>
> In general, we are not allowing more uses of matches(). I think this one
> can be an exception for consistency with the other options, so really
> just a heads up.

The shortening that the matches() allows is very useful for typing. I do
stuff like "ip l sh dev X up" and "ip a a dev X 192.0.2.1/28" all the
time. I suppose there was a discussion about this, can you point me at
the thread, or where & when approximately it took place so I can look it
up?

Re: [PATCH iproute2] dcb: app: Add missing "dcb app show dev X default-prio"

[Stephen Hemminger]

On Wed, 19 Jan 2022 11:38:54 +0100
Petr Machata <petrm@nvidia.com> wrote:

> >
> > In general, we are not allowing more uses of matches(). I think this one
> > can be an exception for consistency with the other options, so really
> > just a heads up.  
> 
> The shortening that the matches() allows is very useful for typing. I do
> stuff like "ip l sh dev X up" and "ip a a dev X 192.0.2.1/28" all the
> time. I suppose there was a discussion about this, can you point me at
> the thread, or where & when approximately it took place so I can look it
> up?

The problem is that matches() doesn't handle conflicts well.
Using your example:
  ip l 
could match "ip link" or "ip l2tp" and the choice of "link" is only because
it was added first. This is bad UI, and creates tribal knowledge that makes
it harder for new users. Other utilities don't allow ambiguous matches.

Re: [PATCH iproute2] dcb: app: Add missing "dcb app show dev X default-prio"

[Stephen Hemminger]

On Tue, 18 Jan 2022 12:36:44 +0100
Petr Machata <petrm@nvidia.com> wrote:

> +		} else if (matches(*argv, "default-prio") == 0) {
> +			dcb_app_print_default_prio(&tab);
>  		} else if (matches(*argv, "dscp-prio") == 0) {
>  			dcb_app_print_dscp_prio(dcb, &tab);

This is an example of why matches() sucks.

Existing users using:
   dcp app show dev X d
will now get different behavior.

You need to redo this patch and put the new argument after "dscp-pri"

Re: [PATCH iproute2] dcb: app: Add missing "dcb app show dev X default-prio"

[David Ahern]

On 1/19/22 1:35 PM, Stephen Hemminger wrote:
> On Wed, 19 Jan 2022 11:38:54 +0100
> Petr Machata <petrm@nvidia.com> wrote:
> 
>>>
>>> In general, we are not allowing more uses of matches(). I think this one
>>> can be an exception for consistency with the other options, so really
>>> just a heads up.  
>>
>> The shortening that the matches() allows is very useful for typing. I do
>> stuff like "ip l sh dev X up" and "ip a a dev X 192.0.2.1/28" all the
>> time. I suppose there was a discussion about this, can you point me at
>> the thread, or where & when approximately it took place so I can look it
>> up?
> 
> The problem is that matches() doesn't handle conflicts well.
> Using your example:
>   ip l 
> could match "ip link" or "ip l2tp" and the choice of "link" is only because
> it was added first. This is bad UI, and creates tribal knowledge that makes
> it harder for new users. Other utilities don't allow ambiguous matches.

and the constant source of bugs when new options are added. This patch
being a good example as Stephen noted.