On 5/27/22 14:45, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Ilya Maximets <i.maxim...@ovn.org>
>> Sent: Friday, May 27, 2022 1:10 PM
>> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Eelco Chaudron
>> <echau...@redhat.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 18:15, Van Haaren, Harry wrote:
>>>> -----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?
>
> <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.maxim...@ovn.org>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
