Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email )
Change subject: PUSH_UPDATE: Added remove_option() and do_update(). ...................................................................... Patch Set 21: Code-Review-1 (4 comments) Patchset: PS21: As discussed on IRC, I do have some things I'd like to see changed before merging. I am taking note of the +2's already given, it's just polishing, comments, readability... File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/20d8d7ae_599aaf68 : PS21, Line 2833: if (!is_update && !check_pull_client_ncp(c, found)) does it make sense to suppress this check? It should never fail, and never modify anything (as we passed on the first run). As it is, this code is a bit confusing, so we'd need a comment ``` /* on PUSH_UPDATE, do not run the NCP checks, because they take 5 minutes * to complete and we are in a hurry */ ``` (put the real reasoning there, of course ;-) ) Mmmh, I think I see the reason, OPT_P_NCP is not set on PUSH_UPDATE, so the code assumes "we have no cipher" and does fallback things... so the comment would need to be something like ``` /* on PUSH_UPDATE, NCP related flags are never updated, and so the code * would assume "no cipher pushed = NCP failed" - so, don't call it on * updates */ ``` File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/758d81ff_703882a6 : PS21, Line 5502: options_server_import(struct options *o, please do not move them (as for 808). thanks :-) http://gerrit.openvpn.net/c/openvpn/+/809/comment/dbc0b2a7_f3d837dd : PS21, Line 3134: } with the extra code, the comment before the function is no longer appropriate. Also, it's unclear to me why this is needed? I'm not guessing the reasoning but claiming "this does not look reasonable" - if `redirect-gateway def1` was pushed or updated, we should not remove it again just because it's not METHOD_SERVICE? If we need it, we definitely need a comment to explain why. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 21 Gerrit-Owner: mrbff <ma...@mandelbit.com> Gerrit-Reviewer: cron2 <g...@greenie.muc.de> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org> Gerrit-Reviewer: stipa <lstipa...@gmail.com> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Attention: mrbff <ma...@mandelbit.com> Gerrit-Comment-Date: Mon, 21 Jul 2025 18:25:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
