[Openvpn-devel] [M] Change in openvpn[master]: Install host routes with onlink scope iroutes for ifconfig-push routes

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

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 <arn...@rf...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: ordex <an...@ma...>
Gerrit-CC: d12fk <he...@op...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 16 Sep 2025 21:19:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment