Message ID | 20200722095446.25141-1-davids@openvpn.net |
---|---|
State | Rejected |
Headers | show |
Series | [Openvpn-devel] options: Remove --udp-mtu | expand |
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
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]);
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(-)