| Message ID | 20220525141014.661907-1-harry.van.haaren@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] dpcls: revert subtable-lookup-prio-get name change | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 25 May 2022, at 16:10, Harry van Haaren wrote: > This commit reverts the name-change that was done (prio->info). > The change breaks a user visible ovs-appctl command, resulting in > breakage of tools/scripts/user-expectation outside of the OVS repo. > > This commit changes the documentation, command string, and unit tests > back to the expected "prio" string, as expected in OVS 2.17 and earlier. Can’t remember all the details, but I guess Ilya’s concern was that the command name does not reflect the actual output. So what about hiding the “subtable-lookup-prio-get” command, and making it such that it still works for backward compatibility? I think this can easily be done by adding the command with a NULL usage string. I guess all that needs to be added is another command, something like, but please test it: + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL, + 0, 0, dpif_netdev_subtable_lookup_get, + NULL); Harry/Ilya what do you think? //Eelco > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > This name change confusion seems to have arisen from the discussion on the v5 version of the patch: > https://patchwork.ozlabs.org/project/openvswitch/patch/20211215041511.4097090-1-kumar.amber@intel.com/ > > --- > Documentation/topics/dpdk/bridge.rst | 4 ++-- > lib/dpif-netdev.c | 2 +- > tests/pmd.at | 16 ++++++++-------- > 3 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst > index 1f626c7c2..314c31a47 100644 > --- a/Documentation/topics/dpdk/bridge.rst > +++ b/Documentation/topics/dpdk/bridge.rst > @@ -179,7 +179,7 @@ these CPU ISA additions are available, and to allow the user to enable them. > OVS provides multiple implementations of dpcls. The following command enables > the user to check what implementations are available in a running instance:: > > - $ ovs-appctl dpif-netdev/subtable-lookup-info-get > + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > Available dpcls implementations: > autovalidator (Use count: 1, Priority: 5) > generic (Use count: 0, Priority: 1) > @@ -195,7 +195,7 @@ above indicates that one subtable of one DPCLS port is has changed its lookup > function due to the command being run. To verify the prioritization, re-run the > get command, note the updated priority of the ``avx512_gather`` function:: > > - $ ovs-appctl dpif-netdev/subtable-lookup-info-get > + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > Available dpcls implementations: > autovalidator (Use count: 1, Priority: 5) > generic (Use count: 0, Priority: 1) > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 0e7a7d16e..ebbd10b24 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -1605,7 +1605,7 @@ dpif_netdev_init(void) > "[lookup_func] [prio]", > 2, 2, dpif_netdev_subtable_lookup_set, > NULL); > - unixctl_command_register("dpif-netdev/subtable-lookup-info-get", "", > + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", "", > 0, 0, dpif_netdev_subtable_lookup_get, > NULL); > unixctl_command_register("dpif-netdev/dpif-impl-set", > diff --git a/tests/pmd.at b/tests/pmd.at > index e6b173dab..df7875c65 100644 > --- a/tests/pmd.at > +++ b/tests/pmd.at > @@ -1130,11 +1130,11 @@ OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0]) > AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd]) > > AT_CHECK([ovs-vsctl show], [], [stdout]) > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl > generic (Use count: 0, Priority: 1) > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep autovalidator], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep autovalidator], [], [dnl > autovalidator (Use count: 0, Priority: 0) > ]) > > @@ -1142,7 +1142,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 3], [0], > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep autovalidator], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep autovalidator], [], [dnl > autovalidator (Use count: 0, Priority: 3) > ]) > > @@ -1150,7 +1150,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 4], [0], [dnl > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl > generic (Use count: 0, Priority: 4) > ]) > > @@ -1158,7 +1158,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 8], [0], [dnl > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl > generic (Use count: 0, Priority: 8) > ]) > > @@ -1166,7 +1166,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 8], [0], > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep autovalidator], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep autovalidator], [], [dnl > autovalidator (Use count: 0, Priority: 8) > ]) > > @@ -1174,7 +1174,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 0], [0], [dnl > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl > generic (Use count: 0, Priority: 0) > ]) > > @@ -1182,7 +1182,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 255], [0], [dn > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl > generic (Use count: 0, Priority: 255) > ]) > > -- > 2.32.0
> This commit reverts the name-change that was done (prio->info). > The change breaks a user visible ovs-appctl command, resulting in > breakage of tools/scripts/user-expectation outside of the OVS repo. > > This commit changes the documentation, command string, and unit tests > back to the expected "prio" string, as expected in OVS 2.17 and earlier. > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > Thanks for the patch Harry. I hadn't thought of this myself and was of the opinion the name change was warranted to reflect the purpose f the command better but I'm of the mind to agree with you now. The command as it was in place for a few releases. @Ilya/@Eelco, what's your opinion, changing the name of the command I guess is something we want to avoid to ensure no impact on users moving between releases? Regards Ian > --- > > This name change confusion seems to have arisen from the discussion on > the v5 version of the patch: > https://patchwork.ozlabs.org/project/openvswitch/patch/2021121504151 > 1.4097090-1-kumar.amber@intel.com/ > > --- > Documentation/topics/dpdk/bridge.rst | 4 ++-- > lib/dpif-netdev.c | 2 +- > tests/pmd.at | 16 ++++++++-------- > 3 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/Documentation/topics/dpdk/bridge.rst > b/Documentation/topics/dpdk/bridge.rst > index 1f626c7c2..314c31a47 100644 > --- a/Documentation/topics/dpdk/bridge.rst > +++ b/Documentation/topics/dpdk/bridge.rst > @@ -179,7 +179,7 @@ these CPU ISA additions are available, and to allow > the user to enable them. > OVS provides multiple implementations of dpcls. The following command > enables > the user to check what implementations are available in a running > instance:: > > - $ ovs-appctl dpif-netdev/subtable-lookup-info-get > + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > Available dpcls implementations: > autovalidator (Use count: 1, Priority: 5) > generic (Use count: 0, Priority: 1) > @@ -195,7 +195,7 @@ above indicates that one subtable of one DPCLS > port is has changed its lookup > function due to the command being run. To verify the prioritization, re-run > the > get command, note the updated priority of the ``avx512_gather`` > function:: > > - $ ovs-appctl dpif-netdev/subtable-lookup-info-get > + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > Available dpcls implementations: > autovalidator (Use count: 1, Priority: 5) > generic (Use count: 0, Priority: 1) > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 0e7a7d16e..ebbd10b24 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -1605,7 +1605,7 @@ dpif_netdev_init(void) > "[lookup_func] [prio]", > 2, 2, dpif_netdev_subtable_lookup_set, > NULL); > - unixctl_command_register("dpif-netdev/subtable-lookup-info-get", "", > + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", "", > 0, 0, dpif_netdev_subtable_lookup_get, > NULL); > unixctl_command_register("dpif-netdev/dpif-impl-set", > diff --git a/tests/pmd.at b/tests/pmd.at > index e6b173dab..df7875c65 100644 > --- a/tests/pmd.at > +++ b/tests/pmd.at > @@ -1130,11 +1130,11 @@ OVS_VSWITCHD_START([], [], [], [--dummy- > numa 0,0]) > AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy- > pmd]) > > AT_CHECK([ovs-vsctl show], [], [stdout]) > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > generic], [], [dnl > generic (Use count: 0, Priority: 1) > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > autovalidator], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > autovalidator], [], [dnl > autovalidator (Use count: 0, Priority: 0) > ]) > > @@ -1142,7 +1142,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > lookup-prio-set autovalidator 3], [0], > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > autovalidator], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > autovalidator], [], [dnl > autovalidator (Use count: 0, Priority: 3) > ]) > > @@ -1150,7 +1150,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > lookup-prio-set generic 4], [0], [dnl > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > generic], [], [dnl > generic (Use count: 0, Priority: 4) > ]) > > @@ -1158,7 +1158,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > lookup-prio-set generic 8], [0], [dnl > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > generic], [], [dnl > generic (Use count: 0, Priority: 8) > ]) > > @@ -1166,7 +1166,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > lookup-prio-set autovalidator 8], [0], > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > autovalidator], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > autovalidator], [], [dnl > autovalidator (Use count: 0, Priority: 8) > ]) > > @@ -1174,7 +1174,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > lookup-prio-set generic 0], [0], [dnl > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > generic], [], [dnl > generic (Use count: 0, Priority: 0) > ]) > > @@ -1182,7 +1182,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > lookup-prio-set generic 255], [0], [dn > Lookup priority change affected 0 dpcls ports and 0 subtables. > ]) > > -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > generic], [], [dnl > +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > generic], [], [dnl > generic (Use count: 0, Priority: 255) > ]) > > -- > 2.32.0
On 25 May 2022, at 16:35, Stokes, Ian wrote: >> This commit reverts the name-change that was done (prio->info). >> The change breaks a user visible ovs-appctl command, resulting in >> breakage of tools/scripts/user-expectation outside of the OVS repo. >> >> This commit changes the documentation, command string, and unit tests >> back to the expected "prio" string, as expected in OVS 2.17 and earlier. >> >> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> >> > > Thanks for the patch Harry. > > I hadn't thought of this myself and was of the opinion the name change was warranted to reflect the purpose f the command better but I'm of the mind to agree with you now. The command as it was in place for a few releases. > > @Ilya/@Eelco, what's your opinion, changing the name of the command I guess is something we want to avoid to ensure no impact on users moving between releases? Guess our emails crossed :) > Regards > Ian > >> --- >> >> This name change confusion seems to have arisen from the discussion on >> the v5 version of the patch: >> https://patchwork.ozlabs.org/project/openvswitch/patch/2021121504151 >> 1.4097090-1-kumar.amber@intel.com/ >> >> --- >> Documentation/topics/dpdk/bridge.rst | 4 ++-- >> lib/dpif-netdev.c | 2 +- >> tests/pmd.at | 16 ++++++++-------- >> 3 files changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/topics/dpdk/bridge.rst >> b/Documentation/topics/dpdk/bridge.rst >> index 1f626c7c2..314c31a47 100644 >> --- a/Documentation/topics/dpdk/bridge.rst >> +++ b/Documentation/topics/dpdk/bridge.rst >> @@ -179,7 +179,7 @@ these CPU ISA additions are available, and to allow >> the user to enable them. >> OVS provides multiple implementations of dpcls. The following command >> enables >> the user to check what implementations are available in a running >> instance:: >> >> - $ ovs-appctl dpif-netdev/subtable-lookup-info-get >> + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get >> Available dpcls implementations: >> autovalidator (Use count: 1, Priority: 5) >> generic (Use count: 0, Priority: 1) >> @@ -195,7 +195,7 @@ above indicates that one subtable of one DPCLS >> port is has changed its lookup >> function due to the command being run. To verify the prioritization, re-run >> the >> get command, note the updated priority of the ``avx512_gather`` >> function:: >> >> - $ ovs-appctl dpif-netdev/subtable-lookup-info-get >> + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get >> Available dpcls implementations: >> autovalidator (Use count: 1, Priority: 5) >> generic (Use count: 0, Priority: 1) >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 0e7a7d16e..ebbd10b24 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -1605,7 +1605,7 @@ dpif_netdev_init(void) >> "[lookup_func] [prio]", >> 2, 2, dpif_netdev_subtable_lookup_set, >> NULL); >> - unixctl_command_register("dpif-netdev/subtable-lookup-info-get", "", >> + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", "", >> 0, 0, dpif_netdev_subtable_lookup_get, >> NULL); >> unixctl_command_register("dpif-netdev/dpif-impl-set", >> diff --git a/tests/pmd.at b/tests/pmd.at >> index e6b173dab..df7875c65 100644 >> --- a/tests/pmd.at >> +++ b/tests/pmd.at >> @@ -1130,11 +1130,11 @@ OVS_VSWITCHD_START([], [], [], [--dummy- >> numa 0,0]) >> AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy- >> pmd]) >> >> AT_CHECK([ovs-vsctl show], [], [stdout]) >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> generic], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> generic], [], [dnl >> generic (Use count: 0, Priority: 1) >> ]) >> >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> autovalidator], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> autovalidator], [], [dnl >> autovalidator (Use count: 0, Priority: 0) >> ]) >> >> @@ -1142,7 +1142,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- >> lookup-prio-set autovalidator 3], [0], >> Lookup priority change affected 0 dpcls ports and 0 subtables. >> ]) >> >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> autovalidator], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> autovalidator], [], [dnl >> autovalidator (Use count: 0, Priority: 3) >> ]) >> >> @@ -1150,7 +1150,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- >> lookup-prio-set generic 4], [0], [dnl >> Lookup priority change affected 0 dpcls ports and 0 subtables. >> ]) >> >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> generic], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> generic], [], [dnl >> generic (Use count: 0, Priority: 4) >> ]) >> >> @@ -1158,7 +1158,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- >> lookup-prio-set generic 8], [0], [dnl >> Lookup priority change affected 0 dpcls ports and 0 subtables. >> ]) >> >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> generic], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> generic], [], [dnl >> generic (Use count: 0, Priority: 8) >> ]) >> >> @@ -1166,7 +1166,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- >> lookup-prio-set autovalidator 8], [0], >> Lookup priority change affected 0 dpcls ports and 0 subtables. >> ]) >> >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> autovalidator], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> autovalidator], [], [dnl >> autovalidator (Use count: 0, Priority: 8) >> ]) >> >> @@ -1174,7 +1174,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- >> lookup-prio-set generic 0], [0], [dnl >> Lookup priority change affected 0 dpcls ports and 0 subtables. >> ]) >> >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> generic], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> generic], [], [dnl >> generic (Use count: 0, Priority: 0) >> ]) >> >> @@ -1182,7 +1182,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- >> lookup-prio-set generic 255], [0], [dn >> Lookup priority change affected 0 dpcls ports and 0 subtables. >> ]) >> >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep >> generic], [], [dnl >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep >> generic], [], [dnl >> generic (Use count: 0, Priority: 255) >> ]) >> >> -- >> 2.32.0
> On 25 May 2022, at 16:35, Stokes, Ian wrote: > > >> This commit reverts the name-change that was done (prio->info). > >> The change breaks a user visible ovs-appctl command, resulting in > >> breakage of tools/scripts/user-expectation outside of the OVS repo. > >> > >> This commit changes the documentation, command string, and unit > tests > >> back to the expected "prio" string, as expected in OVS 2.17 and earlier. > >> > >> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > >> > > > > Thanks for the patch Harry. > > > > I hadn't thought of this myself and was of the opinion the name change > was warranted to reflect the purpose f the command better but I'm of the > mind to agree with you now. The command as it was in place for a few > releases. > > > > @Ilya/@Eelco, what's your opinion, changing the name of the command I > guess is something we want to avoid to ensure no impact on users moving > between releases? > > Guess our emails crossed :) Yes, 😊, apologies, lets carry on with your thread instead of this one. Thanks Ian > > > Regards > > Ian > > > >> --- > >> > >> This name change confusion seems to have arisen from the discussion > on > >> the v5 version of the patch: > >> > https://patchwork.ozlabs.org/project/openvswitch/patch/2021121504151 > >> 1.4097090-1-kumar.amber@intel.com/ > >> > >> --- > >> Documentation/topics/dpdk/bridge.rst | 4 ++-- > >> lib/dpif-netdev.c | 2 +- > >> tests/pmd.at | 16 ++++++++-------- > >> 3 files changed, 11 insertions(+), 11 deletions(-) > >> > >> diff --git a/Documentation/topics/dpdk/bridge.rst > >> b/Documentation/topics/dpdk/bridge.rst > >> index 1f626c7c2..314c31a47 100644 > >> --- a/Documentation/topics/dpdk/bridge.rst > >> +++ b/Documentation/topics/dpdk/bridge.rst > >> @@ -179,7 +179,7 @@ these CPU ISA additions are available, and to > allow > >> the user to enable them. > >> OVS provides multiple implementations of dpcls. The following > command > >> enables > >> the user to check what implementations are available in a running > >> instance:: > >> > >> - $ ovs-appctl dpif-netdev/subtable-lookup-info-get > >> + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > >> Available dpcls implementations: > >> autovalidator (Use count: 1, Priority: 5) > >> generic (Use count: 0, Priority: 1) > >> @@ -195,7 +195,7 @@ above indicates that one subtable of one DPCLS > >> port is has changed its lookup > >> function due to the command being run. To verify the prioritization, re- > run > >> the > >> get command, note the updated priority of the ``avx512_gather`` > >> function:: > >> > >> - $ ovs-appctl dpif-netdev/subtable-lookup-info-get > >> + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > >> Available dpcls implementations: > >> autovalidator (Use count: 1, Priority: 5) > >> generic (Use count: 0, Priority: 1) > >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > >> index 0e7a7d16e..ebbd10b24 100644 > >> --- a/lib/dpif-netdev.c > >> +++ b/lib/dpif-netdev.c > >> @@ -1605,7 +1605,7 @@ dpif_netdev_init(void) > >> "[lookup_func] [prio]", > >> 2, 2, dpif_netdev_subtable_lookup_set, > >> NULL); > >> - unixctl_command_register("dpif-netdev/subtable-lookup-info-get", > "", > >> + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", > "", > >> 0, 0, dpif_netdev_subtable_lookup_get, > >> NULL); > >> unixctl_command_register("dpif-netdev/dpif-impl-set", > >> diff --git a/tests/pmd.at b/tests/pmd.at > >> index e6b173dab..df7875c65 100644 > >> --- a/tests/pmd.at > >> +++ b/tests/pmd.at > >> @@ -1130,11 +1130,11 @@ OVS_VSWITCHD_START([], [], [], [--dummy- > >> numa 0,0]) > >> AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy- > >> pmd]) > >> > >> AT_CHECK([ovs-vsctl show], [], [stdout]) > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> generic], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> generic], [], [dnl > >> generic (Use count: 0, Priority: 1) > >> ]) > >> > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> autovalidator], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> autovalidator], [], [dnl > >> autovalidator (Use count: 0, Priority: 0) > >> ]) > >> > >> @@ -1142,7 +1142,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > >> lookup-prio-set autovalidator 3], [0], > >> Lookup priority change affected 0 dpcls ports and 0 subtables. > >> ]) > >> > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> autovalidator], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> autovalidator], [], [dnl > >> autovalidator (Use count: 0, Priority: 3) > >> ]) > >> > >> @@ -1150,7 +1150,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > >> lookup-prio-set generic 4], [0], [dnl > >> Lookup priority change affected 0 dpcls ports and 0 subtables. > >> ]) > >> > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> generic], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> generic], [], [dnl > >> generic (Use count: 0, Priority: 4) > >> ]) > >> > >> @@ -1158,7 +1158,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > >> lookup-prio-set generic 8], [0], [dnl > >> Lookup priority change affected 0 dpcls ports and 0 subtables. > >> ]) > >> > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> generic], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> generic], [], [dnl > >> generic (Use count: 0, Priority: 8) > >> ]) > >> > >> @@ -1166,7 +1166,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > >> lookup-prio-set autovalidator 8], [0], > >> Lookup priority change affected 0 dpcls ports and 0 subtables. > >> ]) > >> > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> autovalidator], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> autovalidator], [], [dnl > >> autovalidator (Use count: 0, Priority: 8) > >> ]) > >> > >> @@ -1174,7 +1174,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > >> lookup-prio-set generic 0], [0], [dnl > >> Lookup priority change affected 0 dpcls ports and 0 subtables. > >> ]) > >> > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> generic], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> generic], [], [dnl > >> generic (Use count: 0, Priority: 0) > >> ]) > >> > >> @@ -1182,7 +1182,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable- > >> lookup-prio-set generic 255], [0], [dn > >> Lookup priority change affected 0 dpcls ports and 0 subtables. > >> ]) > >> > >> -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep > >> generic], [], [dnl > >> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > >> generic], [], [dnl > >> generic (Use count: 0, Priority: 255) > >> ]) > >> > >> -- > >> 2.32.0
On 5/25/22 16:32, Eelco Chaudron wrote: > > > On 25 May 2022, at 16:10, Harry van Haaren wrote: > >> This commit reverts the name-change that was done (prio->info). >> The change breaks a user visible ovs-appctl command, resulting in >> breakage of tools/scripts/user-expectation outside of the OVS repo. >> >> This commit changes the documentation, command string, and unit tests >> back to the expected "prio" string, as expected in OVS 2.17 and earlier. > > Can’t remember all the details, but I guess Ilya’s concern was that the command name does not reflect the actual output. > > So what about hiding the “subtable-lookup-prio-get” command, and making it such that it still works for backward compatibility? > I think this can easily be done by adding the command with a NULL usage string. > > I guess all that needs to be added is another command, something like, but please test it: > > + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL, > + 0, 0, dpif_netdev_subtable_lookup_get, > + NULL); > > Harry/Ilya what do you think? The unlisted alias looks like a good middle ground to me. The original patch is also missing the NEWS file update that should be added, mentioning the re-name. At the same time, I'm not sure if we actually need to maintain backward compatibility here as this command can hardly be used in automated scripts. The output format is changed significantly, so any automated tools will have to be modified anyway. In general, appctl APIs are supposed to be used mostly by humans, especially "get/show/etc" ones as we're not maintaining the exact output format for them. And if we're not maintaining the output format, there is no much value in maintaining the command name. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Wednesday, May 25, 2022 4:33 PM > To: Eelco Chaudron <echaudro@redhat.com>; Van Haaren, Harry > <harry.van.haaren@intel.com> > Cc: i.maximets@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian > <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> > Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change > > On 5/25/22 16:32, Eelco Chaudron wrote: > > > > > > On 25 May 2022, at 16:10, Harry van Haaren wrote: > > > >> This commit reverts the name-change that was done (prio->info). > >> The change breaks a user visible ovs-appctl command, resulting in > >> breakage of tools/scripts/user-expectation outside of the OVS repo. > >> > >> This commit changes the documentation, command string, and unit tests > >> back to the expected "prio" string, as expected in OVS 2.17 and earlier. > > > > Can’t remember all the details, but I guess Ilya’s concern was that the command > name does not reflect the actual output. > > > > So what about hiding the “subtable-lookup-prio-get” command, and making it such > that it still works for backward compatibility? > > I think this can easily be done by adding the command with a NULL usage string. > > > > I guess all that needs to be added is another command, something like, but please > test it: > > > > + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL, > > + 0, 0, dpif_netdev_subtable_lookup_get, > > + NULL); > > > > Harry/Ilya what do you think? > > The unlisted alias looks like a good middle ground to me. "subtable-lookup-prio-get" lists the various implementations, their use-count, and priority. Sample output: # ./ovs-appctl dpif-netdev/subtable-lookup-prio-get Available dpcls implementations: autovalidator (Use count: 0, Priority: 0) generic (Use count: 0, Priority: 1) avx512_gather (Use count: 2, Priority: 3) That's a pretty good name for a function that gets the priority of the subtable lookup functions. It exists today, users and scripts already use it. Unfortunately its name was changed, and that broke user scripts and command histories. This patch fixes it to be consistent again. > The original patch is also missing the NEWS file update that > should be added, mentioning the re-name. > > At the same time, I'm not sure if we actually need to maintain > backward compatibility here as this command can hardly be used > in automated scripts. The output format is changed significantly, > so any automated tools will have to be modified anyway. Consider a script that sets the autovalidator, runs for 5 seconds, and then turns on the scalar again. It does not need to parse the output, so if we leave the command as before, it will continue to work. If existing scripts were sed/grep/awk parsing out the "priority: value", it will continue to work. If we change the name, we've just broken our users scripts. > In general, appctl APIs are supposed to be used mostly by humans, > especially "get/show/etc" ones as we're not maintaining the exact > output format for them. And if we're not maintaining the output > format, there is no much value in maintaining the command name. There is value in maintaining command-name stability -> see above example. It shouldn't matter if things are supposed to be used specific ways in general, reality is that these commands are put into scripts, and renaming it will break. I don't know what else to say, renaming/breaking commands has no value in my eyes, and only makes using the command difficult. This patch should be merged ASAP, to return the command to the known command string, and fix the current breakage of the user's scripts/command-history. > Best regards, Ilya Maximets. Regards, -Harry
On 5/25/22 18:15, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Wednesday, May 25, 2022 4:33 PM >> To: Eelco Chaudron <echaudro@redhat.com>; Van Haaren, Harry >> <harry.van.haaren@intel.com> >> Cc: i.maximets@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian >> <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> >> Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change >> >> On 5/25/22 16:32, Eelco Chaudron wrote: >>> >>> >>> On 25 May 2022, at 16:10, Harry van Haaren wrote: >>> >>>> This commit reverts the name-change that was done (prio->info). >>>> The change breaks a user visible ovs-appctl command, resulting in >>>> breakage of tools/scripts/user-expectation outside of the OVS repo. >>>> >>>> This commit changes the documentation, command string, and unit tests >>>> back to the expected "prio" string, as expected in OVS 2.17 and earlier. >>> >>> Can’t remember all the details, but I guess Ilya’s concern was that the command >> name does not reflect the actual output. >>> >>> So what about hiding the “subtable-lookup-prio-get” command, and making it such >> that it still works for backward compatibility? >>> I think this can easily be done by adding the command with a NULL usage string. >>> >>> I guess all that needs to be added is another command, something like, but please >> test it: >>> >>> + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL, >>> + 0, 0, dpif_netdev_subtable_lookup_get, >>> + NULL); >>> >>> Harry/Ilya what do you think? >> >> The unlisted alias looks like a good middle ground to me. > > "subtable-lookup-prio-get" lists the various implementations, their use-count, and priority. Sample output: > # ./ovs-appctl dpif-netdev/subtable-lookup-prio-get > Available dpcls implementations: > autovalidator (Use count: 0, Priority: 0) > generic (Use count: 0, Priority: 1) > avx512_gather (Use count: 2, Priority: 3) > > That's a pretty good name for a function that gets the priority of the subtable lookup functions. I know how the output looks like. And, as you said yourself, this command now lists the implementations, hteir use-count and priority. So, the priority is the *third* thing that it lists. The purpose of that command changed, hence the name. > It exists today, users and scripts already use it. Unfortunately its name was changed, and that > broke user scripts and command histories. This patch fixes it to be consistent again. What's wrong with an alias? It preserves the backward compatibility, so there should not be any problems. > > >> The original patch is also missing the NEWS file update that >> should be added, mentioning the re-name. >> >> At the same time, I'm not sure if we actually need to maintain >> backward compatibility here as this command can hardly be used >> in automated scripts. The output format is changed significantly, >> so any automated tools will have to be modified anyway. > > Consider a script that sets the autovalidator, runs for 5 seconds, and then turns on the scalar again. > It does not need to parse the output, so if we leave the command as before, it will continue to work. > If existing scripts were sed/grep/awk parsing out the "priority: value", it will continue to work. > If we change the name, we've just broken our users scripts. Sure, but we're not changing the name of the 'set' command. > > >> In general, appctl APIs are supposed to be used mostly by humans, >> especially "get/show/etc" ones as we're not maintaining the exact >> output format for them. And if we're not maintaining the output >> format, there is no much value in maintaining the command name. > > There is value in maintaining command-name stability -> see above example. > It shouldn't matter if things are supposed to be used specific ways in general, > reality is that these commands are put into scripts, and renaming it will break. > > I don't know what else to say, renaming/breaking commands has no value in my eyes, and only > makes using the command difficult. > > This patch should be merged ASAP, to return the command to the known command string, > and fix the current breakage of the user's scripts/command-history. Again, what's wrong with the backward compatible alias? > > >> Best regards, Ilya Maximets. > > Regards, -Harry
On 5/27/22 14:09, Ilya Maximets wrote: > On 5/25/22 18:15, Van Haaren, Harry wrote: >>> -----Original Message----- >>> From: Ilya Maximets <i.maximets@ovn.org> >>> Sent: Wednesday, May 25, 2022 4:33 PM >>> To: Eelco Chaudron <echaudro@redhat.com>; Van Haaren, Harry >>> <harry.van.haaren@intel.com> >>> Cc: i.maximets@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian >>> <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> >>> Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change >>> >>> On 5/25/22 16:32, Eelco Chaudron wrote: >>>> >>>> >>>> On 25 May 2022, at 16:10, Harry van Haaren wrote: >>>> >>>>> This commit reverts the name-change that was done (prio->info). >>>>> The change breaks a user visible ovs-appctl command, resulting in >>>>> breakage of tools/scripts/user-expectation outside of the OVS repo. >>>>> >>>>> This commit changes the documentation, command string, and unit tests >>>>> back to the expected "prio" string, as expected in OVS 2.17 and earlier. >>>> >>>> Can’t remember all the details, but I guess Ilya’s concern was that the command >>> name does not reflect the actual output. >>>> >>>> So what about hiding the “subtable-lookup-prio-get” command, and making it such >>> that it still works for backward compatibility? >>>> I think this can easily be done by adding the command with a NULL usage string. >>>> >>>> I guess all that needs to be added is another command, something like, but please >>> test it: >>>> >>>> + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL, >>>> + 0, 0, dpif_netdev_subtable_lookup_get, >>>> + NULL); >>>> >>>> Harry/Ilya what do you think? >>> >>> The unlisted alias looks like a good middle ground to me. >> >> "subtable-lookup-prio-get" lists the various implementations, their use-count, and priority. Sample output: >> # ./ovs-appctl dpif-netdev/subtable-lookup-prio-get >> Available dpcls implementations: >> autovalidator (Use count: 0, Priority: 0) >> generic (Use count: 0, Priority: 1) >> avx512_gather (Use count: 2, Priority: 3) >> >> That's a pretty good name for a function that gets the priority of the subtable lookup functions. > > I know how the output looks like. And, as you said yourself, > this command now lists the implementations, hteir use-count > and priority. So, the priority is the *third* thing that it > lists. The purpose of that command changed, hence the name. > >> It exists today, users and scripts already use it. Unfortunately its name was changed, and that >> broke user scripts and command histories. This patch fixes it to be consistent again. > > What's wrong with an alias? It preserves the backward compatibility, > so there should not be any problems. > >> >> >>> The original patch is also missing the NEWS file update that >>> should be added, mentioning the re-name. >>> >>> At the same time, I'm not sure if we actually need to maintain >>> backward compatibility here as this command can hardly be used >>> in automated scripts. The output format is changed significantly, >>> so any automated tools will have to be modified anyway. >> >> Consider a script that sets the autovalidator, runs for 5 seconds, and then turns on the scalar again. >> It does not need to parse the output, so if we leave the command as before, it will continue to work. >> If existing scripts were sed/grep/awk parsing out the "priority: value", it will continue to work. For the record, that is not true. The old format was '<value>: name', so any sed/grep/awk will fail to parse the new format, which is 'Priority: <value>'. >> If we change the name, we've just broken our users scripts. > > Sure, but we're not changing the name of the 'set' command. > >> >> >>> In general, appctl APIs are supposed to be used mostly by humans, >>> especially "get/show/etc" ones as we're not maintaining the exact >>> output format for them. And if we're not maintaining the output >>> format, there is no much value in maintaining the command name. >> >> There is value in maintaining command-name stability -> see above example. >> It shouldn't matter if things are supposed to be used specific ways in general, >> reality is that these commands are put into scripts, and renaming it will break. >> >> I don't know what else to say, renaming/breaking commands has no value in my eyes, and only >> makes using the command difficult. >> >> This patch should be merged ASAP, to return the command to the known command string, >> and fix the current breakage of the user's scripts/command-history. > > Again, what's wrong with the backward compatible alias? > >> >> >>> Best regards, Ilya Maximets. >> >> Regards, -Harry
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Friday, May 27, 2022 1:10 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com>; Eelco Chaudron > <echaudro@redhat.com> > Cc: i.maximets@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian > <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> > Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change > > On 5/25/22 18:15, Van Haaren, Harry wrote: > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Wednesday, May 25, 2022 4:33 PM > >> To: Eelco Chaudron <echaudro@redhat.com>; Van Haaren, Harry > >> <harry.van.haaren@intel.com> > >> Cc: i.maximets@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian > >> <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> > >> Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change > >> > >> On 5/25/22 16:32, Eelco Chaudron wrote: > >>> > >>> > >>> On 25 May 2022, at 16:10, Harry van Haaren wrote: > >>> > >>>> This commit reverts the name-change that was done (prio->info). > >>>> The change breaks a user visible ovs-appctl command, resulting in > >>>> breakage of tools/scripts/user-expectation outside of the OVS repo. > >>>> > >>>> This commit changes the documentation, command string, and unit tests > >>>> back to the expected "prio" string, as expected in OVS 2.17 and earlier. > >>> > >>> Can’t remember all the details, but I guess Ilya’s concern was that the command > >> name does not reflect the actual output. > >>> > >>> So what about hiding the “subtable-lookup-prio-get” command, and making it > such > >> that it still works for backward compatibility? > >>> I think this can easily be done by adding the command with a NULL usage string. > >>> > >>> I guess all that needs to be added is another command, something like, but > please > >> test it: > >>> > >>> + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL, > >>> + 0, 0, dpif_netdev_subtable_lookup_get, > >>> + NULL); > >>> > >>> Harry/Ilya what do you think? <snip away discussion, trying to focus on a solution> > > There is value in maintaining command-name stability -> see above example. > > It shouldn't matter if things are supposed to be used specific ways in general, > > reality is that these commands are put into scripts, and renaming it will break. > > > > I don't know what else to say, renaming/breaking commands has no value in my > eyes, and only > > makes using the command difficult. > > > > This patch should be merged ASAP, to return the command to the known command > string, > > and fix the current breakage of the user's scripts/command-history. > > Again, what's wrong with the backward compatible alias? For bug fix patches, or resolving issues I tend to prefer solution that offers the lowest-risk, and least amount of unknown/new changes. I think this patch achieves that. The backwards compatible alias is a "new/different" solution, and hence not my preferred option. I'll leave it to you as OVS maintainer; there's a fix patch here I'm confident in, and can be merged. If there is a preference to develop and merge an alternative solution, that's Ok with me too, but I won't commit to spending time on it as there is a fix here already.
On 5/27/22 14:45, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Friday, May 27, 2022 1:10 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Eelco Chaudron >> <echaudro@redhat.com> >> Cc: i.maximets@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian >> <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> >> Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change >> >> On 5/25/22 18:15, Van Haaren, Harry wrote: >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maximets@ovn.org> >>>> Sent: Wednesday, May 25, 2022 4:33 PM >>>> To: Eelco Chaudron <echaudro@redhat.com>; Van Haaren, Harry >>>> <harry.van.haaren@intel.com> >>>> Cc: i.maximets@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian >>>> <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> >>>> Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change >>>> >>>> On 5/25/22 16:32, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 25 May 2022, at 16:10, Harry van Haaren wrote: >>>>> >>>>>> This commit reverts the name-change that was done (prio->info). >>>>>> The change breaks a user visible ovs-appctl command, resulting in >>>>>> breakage of tools/scripts/user-expectation outside of the OVS repo. >>>>>> >>>>>> This commit changes the documentation, command string, and unit tests >>>>>> back to the expected "prio" string, as expected in OVS 2.17 and earlier. >>>>> >>>>> Can’t remember all the details, but I guess Ilya’s concern was that the command >>>> name does not reflect the actual output. >>>>> >>>>> So what about hiding the “subtable-lookup-prio-get” command, and making it >> such >>>> that it still works for backward compatibility? >>>>> I think this can easily be done by adding the command with a NULL usage string. >>>>> >>>>> I guess all that needs to be added is another command, something like, but >> please >>>> test it: >>>>> >>>>> + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL, >>>>> + 0, 0, dpif_netdev_subtable_lookup_get, >>>>> + NULL); >>>>> >>>>> Harry/Ilya what do you think? > > <snip away discussion, trying to focus on a solution> > >>> There is value in maintaining command-name stability -> see above example. >>> It shouldn't matter if things are supposed to be used specific ways in general, >>> reality is that these commands are put into scripts, and renaming it will break. >>> >>> I don't know what else to say, renaming/breaking commands has no value in my >> eyes, and only >>> makes using the command difficult. >>> >>> This patch should be merged ASAP, to return the command to the known command >> string, >>> and fix the current breakage of the user's scripts/command-history. >> >> Again, what's wrong with the backward compatible alias? > > For bug fix patches, or resolving issues I tend to prefer solution that offers the lowest-risk, > and least amount of unknown/new changes. I think this patch achieves that. The backwards > compatible alias is a "new/different" solution, and hence not my preferred option. The lowest risk is to just revert the 738c76a503f4. I don't see any serious risk in having an alias, it's not a rocket science. And it's not like we have to fix this ASAP, we have time to develop a good solution. Eelco is basically already provided all the code needed, the code just needs a round of simple testing. > I'll leave it to you as OVS maintainer; there's a fix patch here I'm confident in, and can be merged. > If there is a preference to develop and merge an alternative solution, that's Ok with me too, If someone else wants to pick the change up and submit a new version, that's fine. We may revert the whole 738c76a503f4 as an alternative and go with the dp-extra-info approach instead, that is also fine, even though the format in the 738c76a503f4 suits the purpose a bit better. > but I won't commit to spending time on it as there is a fix here already. The current version of the patch has been reviewed and changes have been requested. This is the standard process. The current version of the change doesn't make sense to me: NACKed-by: Ilya Maximets <i.maximets@ovn.org>
diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst index 1f626c7c2..314c31a47 100644 --- a/Documentation/topics/dpdk/bridge.rst +++ b/Documentation/topics/dpdk/bridge.rst @@ -179,7 +179,7 @@ these CPU ISA additions are available, and to allow the user to enable them. OVS provides multiple implementations of dpcls. The following command enables the user to check what implementations are available in a running instance:: - $ ovs-appctl dpif-netdev/subtable-lookup-info-get + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get Available dpcls implementations: autovalidator (Use count: 1, Priority: 5) generic (Use count: 0, Priority: 1) @@ -195,7 +195,7 @@ above indicates that one subtable of one DPCLS port is has changed its lookup function due to the command being run. To verify the prioritization, re-run the get command, note the updated priority of the ``avx512_gather`` function:: - $ ovs-appctl dpif-netdev/subtable-lookup-info-get + $ ovs-appctl dpif-netdev/subtable-lookup-prio-get Available dpcls implementations: autovalidator (Use count: 1, Priority: 5) generic (Use count: 0, Priority: 1) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 0e7a7d16e..ebbd10b24 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1605,7 +1605,7 @@ dpif_netdev_init(void) "[lookup_func] [prio]", 2, 2, dpif_netdev_subtable_lookup_set, NULL); - unixctl_command_register("dpif-netdev/subtable-lookup-info-get", "", + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", "", 0, 0, dpif_netdev_subtable_lookup_get, NULL); unixctl_command_register("dpif-netdev/dpif-impl-set", diff --git a/tests/pmd.at b/tests/pmd.at index e6b173dab..df7875c65 100644 --- a/tests/pmd.at +++ b/tests/pmd.at @@ -1130,11 +1130,11 @@ OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0]) AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd]) AT_CHECK([ovs-vsctl show], [], [stdout]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl generic (Use count: 0, Priority: 1) ]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep autovalidator], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep autovalidator], [], [dnl autovalidator (Use count: 0, Priority: 0) ]) @@ -1142,7 +1142,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 3], [0], Lookup priority change affected 0 dpcls ports and 0 subtables. ]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep autovalidator], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep autovalidator], [], [dnl autovalidator (Use count: 0, Priority: 3) ]) @@ -1150,7 +1150,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 4], [0], [dnl Lookup priority change affected 0 dpcls ports and 0 subtables. ]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl generic (Use count: 0, Priority: 4) ]) @@ -1158,7 +1158,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 8], [0], [dnl Lookup priority change affected 0 dpcls ports and 0 subtables. ]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl generic (Use count: 0, Priority: 8) ]) @@ -1166,7 +1166,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 8], [0], Lookup priority change affected 0 dpcls ports and 0 subtables. ]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep autovalidator], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep autovalidator], [], [dnl autovalidator (Use count: 0, Priority: 8) ]) @@ -1174,7 +1174,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 0], [0], [dnl Lookup priority change affected 0 dpcls ports and 0 subtables. ]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl generic (Use count: 0, Priority: 0) ]) @@ -1182,7 +1182,7 @@ AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 255], [0], [dn Lookup priority change affected 0 dpcls ports and 0 subtables. ]) -AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-info-get | grep generic], [], [dnl +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl generic (Use count: 0, Priority: 255) ])
This commit reverts the name-change that was done (prio->info). The change breaks a user visible ovs-appctl command, resulting in breakage of tools/scripts/user-expectation outside of the OVS repo. This commit changes the documentation, command string, and unit tests back to the expected "prio" string, as expected in OVS 2.17 and earlier. Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> --- This name change confusion seems to have arisen from the discussion on the v5 version of the patch: https://patchwork.ozlabs.org/project/openvswitch/patch/20211215041511.4097090-1-kumar.amber@intel.com/ --- Documentation/topics/dpdk/bridge.rst | 4 ++-- lib/dpif-netdev.c | 2 +- tests/pmd.at | 16 ++++++++-------- 3 files changed, 11 insertions(+), 11 deletions(-)