[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

cron2 has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

Patchset:

PS1: 
I don't think the logic is right now (neither before nor afterwards).  The commit message actually speaks about `link-mtu` but the code tests `tun_mtu_defined`.


File src/openvpn/push.c:

http://gerrit.openvpn.net/c/openvpn/+/1321/comment/d77bec25_18f7eaaa?usp=email :
PS1, Line 1046:         if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
I don't think this is actually correct - `--tun-mtu` is the *inner* MTU, and that never changes on `--cipher`.  So the original variant means "if we have a configured tun-mtu, a cipher change will not affect this, so we don't care".

A more clear expression would be "if a `--link-mtu` was defined and `--cipher changes` (because in that case, `--tun-mtu` would be recalculated - or would it?)

Maybe that whole thing needs to go after the big mtu refactoring?



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 17:32:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes