> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Friday, May 27, 2022 1:10 PM > To: Van Haaren, Harry <[email protected]>; Eelco Chaudron > <[email protected]> > Cc: [email protected]; [email protected]; Stokes, Ian > <[email protected]>; Amber, Kumar <[email protected]> > 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 <[email protected]> > >> Sent: Wednesday, May 25, 2022 4:33 PM > >> To: Eelco Chaudron <[email protected]>; Van Haaren, Harry > >> <[email protected]> > >> Cc: [email protected]; [email protected]; Stokes, Ian > >> <[email protected]>; Amber, Kumar <[email protected]> > >> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
