| Message ID | f6e07ca31e33a673f641c9282e81ee9c3be03d3c.1642505737.git.petrm@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | [iproute2] dcb: app: Add missing "dcb app show dev X default-prio" | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
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?
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.
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"
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.
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) {