Attention is currently required from: flichtenheld, plaisthos. ordex has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1192?usp=email )
Change subject: Install host routes with onlink scope iroutes for ifconfig-push routes ...................................................................... Patch Set 4: Code-Review+1 (12 comments) Commit Message: http://gerrit.openvpn.net/c/openvpn/+/1192/comment/6b060a08_b9b15038 : PS4, Line 7: Install host routes with onlink scope iroutes for ifconfig-push routes to be honest, this title feels a bit cryptic 😄 maybe it can be simplified a little, since we have the longer description below? http://gerrit.openvpn.net/c/openvpn/+/1192/comment/955bcbc5_de33bffb : PS4, Line 10: of the configured device need to be added to the operating system to the subject for "need" is "Additional IP addresses", but I think you meant "a route to those IPs", right? So this sentence should be massaged a little. Moreover, is `ifconfig-push` really creating an "Additional IP" or is it just substituting the IP assigned out of the server pool? http://gerrit.openvpn.net/c/openvpn/+/1192/comment/9e07abea_3c127d4d : PS4, Line 15: iroute, will not work. should we add "because the server does not have an address in the same network as these IPs assigned to the VPN interface" (some something alike)? Just to make sure in 5 months from now we recall why we did this. Patchset: PS4: patch looks good to me. I just have a few minor nitpicks.. File doc/man-sections/server-options.rst: http://gerrit.openvpn.net/c/openvpn/+/1192/comment/a86c4865_793f1207 : PS4, Line 318: address s/for/to/ s/ /./ http://gerrit.openvpn.net/c/openvpn/+/1192/comment/bff8681a_b8c91a88 : PS4, Line 330: When DCO is enabled, OpenVPN will install a /128 route for the local IP s/for/to/ File src/openvpn/dco.c: http://gerrit.openvpn.net/c/openvpn/+/1192/comment/d9ae49b2_0ce9bbcd : PS4, Line 729: /* Check if we added the local network as /32 route as it was not in should we call it "client network", as "local" seems to be something local to the server File src/openvpn/multi.h: http://gerrit.openvpn.net/c/openvpn/+/1192/comment/c67d489a_5aa0fbc8 : PS4, Line 672: * forgot to document "mi" File src/openvpn/multi.c: http://gerrit.openvpn.net/c/openvpn/+/1192/comment/d08232ec_d390068e : PS4, Line 1251: msg(D_MULTI_LOW, "MULTI: Adding /32 on-link route for route %s -> %s", s/for route/for ifconfig-push/ ? (like you wrote in the ipv6 case) http://gerrit.openvpn.net/c/openvpn/+/1192/comment/bed1bbae_c70f9021 : PS4, Line 1262: if (!primary) for clarity sake, shouldn't this be an "else if"? >From a functional perspective it shouldn't make a difference. http://gerrit.openvpn.net/c/openvpn/+/1192/comment/8760fe93_46153291 : PS4, Line 1315: if (!primary) same http://gerrit.openvpn.net/c/openvpn/+/1192/comment/4a7ec029_5c48f9ed : PS4, Line 4353: */ I think we normally put the doxygen in one place only (i.e. header file), so this can go. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1192?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: I83295e00d1a756dfa44050b0a4493095fb050fff Gerrit-Change-Number: 1192 Gerrit-PatchSet: 4 Gerrit-Owner: plaisthos <[email protected]> Gerrit-Reviewer: flichtenheld <[email protected]> Gerrit-Reviewer: ordex <[email protected]> Gerrit-CC: d12fk <[email protected]> Gerrit-CC: openvpn-devel <[email protected]> Gerrit-Attention: plaisthos <[email protected]> Gerrit-Attention: flichtenheld <[email protected]> Gerrit-Comment-Date: Tue, 16 Sep 2025 21:19:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
