[Openvpn-devel] options: Remove --udp-mtu

Before --link-mtu, it was --udp-mtu.  This was changed in
OpenVPN 1.5_beta1 (release July 2003).  It should be safe now
to remove --udp-mtu, the transition period should have been long
enough.

Signed-off-by: David Sommerseth <davids@openvpn.net>
---
 src/openvpn/options.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Am 22.07.20 um 11:54 schrieb David Sommerseth:
> Before --link-mtu, it was --udp-mtu.  This was changed in
> OpenVPN 1.5_beta1 (release July 2003).  It should be safe now
> to remove --udp-mtu, the transition period should have been long
> enough.
> 
> Signed-off-by: David Sommerseth <davids@openvpn.net>
> ---
>  src/openvpn/options.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index b1962ca4..3ebf033d 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3645,7 +3645,6 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
>   * --dev tun|tap [unit number need not match]
>   * --dev-type tun|tap
>   * --link-mtu
> - * --udp-mtu
>   * --tun-mtu
>   * --proto udp
>   * --proto tcp-client [matched with --proto tcp-server
> @@ -6007,7 +6006,7 @@ add_option(struct options *options,
>              goto err;
>          }
>      }
> -    else if ((streq(p[0], "link-mtu") || streq(p[0], "udp-mtu")) && p[1] && !p[2])
> +    else if (streq(p[0], "link-mtu") && p[1] && !p[2])
>      {
>          VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
>          options->ce.link_mtu = positive_atoi(p[1]);
> 

I close to sending a NAK to this. Normally we just don't outright remove
options but rather warn for a version or two. But adding the warning
code here and later removing the option just to remove the complexity of
'|| streq(p[0], "udp-mtu"'?

I don't see a compelling reason to remove alias option. Normally we
deprecate options because they

- are considered harmful in some way (no-iv)
- complicate the code without a clear gain
- are replaced by a better mechanism and the old option does not
trivally translate to the new option.
- are features that we do no longer want to maintain and to remove them
to decrease complexity (--enable-client-only)

But with alias I feel we just removing them because we found a newer
nicer name and as a user (especially another dev) removing an alias
feels like they are removed because of pride/principle that since they
are old they must be go away. I cannot came up with any other compelling
reason why we need to remove them. And as much as I want to have old
options gone as well, if my only reason is pride/they are old then I
have to say that they should stay while the option they alias also stays.

Arne

On 22/07/2020 14:01, Arne Schwabe wrote:
> Am 22.07.20 um 11:54 schrieb David Sommerseth:
>> Before --link-mtu, it was --udp-mtu.  This was changed in
>> OpenVPN 1.5_beta1 (release July 2003).  It should be safe now
>> to remove --udp-mtu, the transition period should have been long
>> enough.
>>
>> Signed-off-by: David Sommerseth <davids@openvpn.net>
>> ---
>>  src/openvpn/options.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)

[...snip...]

>
> I close to sending a NAK to this. Normally we just don't outright remove
> options but rather warn for a version or two. But adding the warning
> code here and later removing the option just to remove the complexity of
> '|| streq(p[0], "udp-mtu"'?

I don't follow your logic, really.  But I won't fight this much.  My reasons
for removing this so easily are:

 -  This option has not been in any documentation since OpenVPN 2.0_beta,
    released  in November 2004.

 -  It got superseded by --link-mtu in OpenVPN 1.5_beta1 (July 2003)

I would really like to see that configuration file being in active use on a
system today.  OpenVPN 1.x generation was plain P2P mode capable, OpenVPN 2.0
was the first release enabling multiple clients to the same server process.

> I don't see a compelling reason to remove alias option. Normally we
> deprecate options because they
> 
> - are considered harmful in some way (no-iv)
> - complicate the code without a clear gain
> - are replaced by a better mechanism and the old option does not
> trivally translate to the new option.
> - are features that we do no longer want to maintain and to remove them
> to decrease complexity (--enable-client-only)

That is right.  And all of that falls into the category of "code creep", where
the code grows and requires maintenance.

But I see this from the angle of "option creep", where we add options and
never really clean up.  Which is why we have started to deprecate options.
Deprecating options due to code creep is one aspect.  But never clean up
options which has been deprecated or superseded in one way or another is, in
my view, equally bad to not removing "code creep".

I do agree we normally would have a deprecation process going for all options.
 But the --udp-mtu option falls into a very special category; being deprecated
almost 17 years ago and being removed from the documentation almost 16 years
ago + being superceded in an OpenVPN release generation which introduced TCP
support - all of this more than 16 years ago.

Over the years I have also criticized introduction of new options, trying to
reuse existing options as much as possible and not introduce new ones without
a good reason [0].  Our options.c file tackles close to 300 options [1]; where
we have documented 275 of them in our man page.  This is also not good, from a
user's perspective.

We might need a lot of options to handle the flexibility of OpenVPN.  But we
should really strive to not have overlapping options active for too long, as
that is just driving things more complex for the users - without good reason.
 And we certainly should avoid having multiple options doing exactly the same
thing; this is acceptable to me only in a transition phase where we move to a
better construct.

[0] For example, I questioned why we needed --tls-crypt-v2 instead of
    reusing --tls-crypt
[1] $ git grep 'streq(p\[0\],' -- src/openvpn/options.c | wc -l
[2] $ git grep -E "^--\w+" -- doc/man-sections/*.rst | wc -l


> But with alias I feel we just removing them because we found a newer
> nicer name and as a user (especially another dev) removing an alias
> feels like they are removed because of pride/principle that since they
> are old they must be go away. 

This is really not my motivation, I do reject this sentiment and implicitly
pointing it like that towards me.  Lets keep the discussion on the technical
side, not imply anything on the personal side.

What I do believe in is our role of maintaining this project.  Which means we
need to clean up.  Some clean-ups are big.  Some are small.  This patch is a
small one.  Some times we keep options, some times we kick some out, some
times we migrate them to better alternatives.

> 
> 
>> But with alias I feel we just removing them because we found a newer
>> nicer name and as a user (especially another dev) removing an alias
>> feels like they are removed because of pride/principle that since they
>> are old they must be go away. 
> 
> This is really not my motivation, I do reject this sentiment and implicitly
> pointing it like that towards me.  Lets keep the discussion on the technical
> side, not imply anything on the personal side.

Sorry, this was not meant to be on a personal level. I was more in the
idea commenting that it might looks like that this being the main
motivation since I find the other reasons not convincing. And external
people external who will look it up and find the only reason in the git
log that we added this alias 17 years ago and now remove it because it
is old might get exactly the same sentiment.

> What I do believe in is our role of maintaining this project.  Which means we
> need to clean up.  Some clean-ups are big.  Some are small.  This patch is a
> small one.  Some times we keep options, some times we kick some out, some
> times we migrate them to better alternatives.

Yes. I would argue that here the benefit of keeping the option is small
but the benefit of cleaning up is even smaller, so it should stay.

Arne