> -----Original Message-----
> From: Ilya Maximets <i.maxim...@ovn.org>
> Sent: Wednesday, May 25, 2022 4:33 PM
> To: Eelco Chaudron <echau...@redhat.com>; Van Haaren, Harry
> <harry.van.haa...@intel.com>
> Cc: i.maxim...@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian
> <ian.sto...@intel.com>; Amber, Kumar <kumar.am...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
