| Message ID | 20211207170211.3275837-7-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Big buffer/frame refactoring patch set | expand |
> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben: > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 8cbb129d2..303e3fe8f 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -1897,7 +1897,7 @@ tls_session_update_crypto_params_do_work(struct tls_session *session, > frame_print(frame, D_MTU_INFO, "Data Channel MTU parms"); > > /* > - * mssfix uses data channel framing, which at this point contains > + * mssfix uses data channel framing, which at this poipnt contains Spurious change Regards, Frank -- Frank Lichtenheld
> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben: > This code is probably from a time when we could not set the MTU on > the Windows tap6 driver. Nowadays we can set the MTU on this device, > so this code is a noop now. [...] > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > index 75d5eaf7b..12bdd2005 100644 > --- a/src/openvpn/tun.c > +++ b/src/openvpn/tun.c > @@ -6071,7 +6071,6 @@ tuntap_get_mtu(struct tuntap *tt) > &mtu, sizeof(mtu), > &mtu, sizeof(mtu), &len, NULL)) > { > - tt->post_open_mtu = (int)mtu; > msg(D_MTU_INFO, "TAP-Windows MTU=%d", (int)mtu); > } > } This change makes no sense to me. Why would you not remove the complete tuntap_get_mtu call? If the whole point was to get the MTU, and now it will be the one we set, why even to the ioctl at all? Regards, Frank -- Frank Lichtenheld
Am 10.12.21 um 17:17 schrieb Frank Lichtenheld: >> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben: >> This code is probably from a time when we could not set the MTU on >> the Windows tap6 driver. Nowadays we can set the MTU on this device, >> so this code is a noop now. > [...] >> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c >> index 75d5eaf7b..12bdd2005 100644 >> --- a/src/openvpn/tun.c >> +++ b/src/openvpn/tun.c >> @@ -6071,7 +6071,6 @@ tuntap_get_mtu(struct tuntap *tt) >> &mtu, sizeof(mtu), >> &mtu, sizeof(mtu), &len, NULL)) >> { >> - tt->post_open_mtu = (int)mtu; >> msg(D_MTU_INFO, "TAP-Windows MTU=%d", (int)mtu); >> } >> } > > This change makes no sense to me. Why would you not remove the complete tuntap_get_mtu call? If the whole point was to get the MTU, and now it will be the one we set, why even to the ioctl at all? I wanted to keep the message that is currently printed at verb 1. If we decide that the message can be removed I can remove the calling of the ioctl as well. Arne
Acked-by: Gert Doering <gert@greenie.muc.de> I follow the reasoning - since we set the MTU now, we do not need to adjust the frame afterwards. Logging the MTU might be useful (Windows is weird, maybe it does not always "stick"...), so let's keep this in for the time being. Spurious comment change in ssl.c removed from the patch. I like the comment /* Some TUN/TAP drivers like to be ioctled for mtu after open */ "like to be ioctled", hah :-) - but why this is there is lost lore from the long gone past... I have test compiled this on mingw. Your patch has been applied to the master branch. commit c27868bfc2ad6ee21c28f4addb1c2fc003bfa61a Author: Arne Schwabe Date: Tue Dec 7 18:01:56 2021 +0100 Remove post_open_mtu code Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20211207170211.3275837-7-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23327.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 0009bcb72..85d664ad6 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1809,16 +1809,6 @@ do_open_tun(struct context *c) c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); } - /* - * Did tun/tap driver give us an MTU? - */ - if (c->c1.tuntap->post_open_mtu) - { - frame_set_mtu_dynamic(&c->c2.frame, - c->c1.tuntap->post_open_mtu, - SET_MTU_TUN | SET_MTU_UPPER_BOUND); - } - ret = true; static_context = c; #ifndef TARGET_ANDROID diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 8cbb129d2..303e3fe8f 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1897,7 +1897,7 @@ tls_session_update_crypto_params_do_work(struct tls_session *session, frame_print(frame, D_MTU_INFO, "Data Channel MTU parms"); /* - * mssfix uses data channel framing, which at this point contains + * mssfix uses data channel framing, which at this poipnt contains * actual overhead. Fragmentation logic uses frame_fragment, which * still contains worst case overhead. Replace it with actual overhead * to prevent unneeded fragmentation. diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 75d5eaf7b..12bdd2005 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -6071,7 +6071,6 @@ tuntap_get_mtu(struct tuntap *tt) &mtu, sizeof(mtu), &mtu, sizeof(mtu), &len, NULL)) { - tt->post_open_mtu = (int)mtu; msg(D_MTU_INFO, "TAP-Windows MTU=%d", (int)mtu); } } diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index aa1e47b5a..d4657537c 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -214,10 +214,6 @@ struct tuntap #endif /* used for printing status info only */ unsigned int rwflags_debug; - - /* Some TUN/TAP drivers like to be ioctled for mtu - * after open */ - int post_open_mtu; }; static inline bool
This code is probably from a time when we could not set the MTU on the Windows tap6 driver. Nowadays we can set the MTU on this device, so this code is a noop now. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/init.c | 10 ---------- src/openvpn/ssl.c | 2 +- src/openvpn/tun.c | 1 - src/openvpn/tun.h | 4 ---- 4 files changed, 1 insertion(+), 16 deletions(-)