Attention is currently required from: flichtenheld, plaisthos. ralf_lici has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/739?usp=email )
Change subject: Support CIDR on options and extend netbits usage ...................................................................... Patch Set 3: (25 comments) Commit Message: http://gerrit.openvpn.net/c/openvpn/+/739/comment/ef955895_c40062c5 : PS3, Line 9: Add support for CIDR notation on all suitable options (client-nat, > Maybe give some more motivation here. […] Acknowledged File doc/man-sections/server-options.rst: http://gerrit.openvpn.net/c/openvpn/+/739/comment/36991047_c59736fd : PS3, Line 344: :code:`255.255.255.255`. > for completeness mentions /32 as well. Acknowledged File doc/man-sections/vpn-network-options.rst: http://gerrit.openvpn.net/c/openvpn/+/739/comment/61768e6b_7acc98ad : PS3, Line 220: ifconfig local rn > Need to specify the two possibilities (IP address or netmask) separately. […] I think it stands for remote_netmask. I left it like this because I found it mentioned in this form in the explanation that follows (line 237) but I agree it isn't clear. I'm duplicating it to show both versions of ifconfig (remote and netmask) as you suggested. However this may not be completely coherent to --ifconfig-push (which has "local remote-netmask" arguments). Should I change that one too accordingly? http://gerrit.openvpn.net/c/openvpn/+/739/comment/308f35e0_f16ee639 : PS3, Line 266: 2 255 > Add /bits example here? Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/1861ac78_b520ac5c : PS3, Line 389: route network|ipv4addr [netmask] [gateway] [metric] > Does the network|ipv4addr here really add any value? Should we go just for > ipv4addr here like with - […] I didn't drop the "network" format because I thought it was referring to the DNS resolvable name or special keywords as opposed to the ipv4 address. But I see that it can be a bit misleading therefore I will duplicate the lines (network and ipv4addr). File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/ce30cce3_6ee36c90 : PS3, Line 6103: if (!p[2]) /* p[1] must be in CIDR format */ > Should probably do a ip_or_dns_addr_safe(p[1]) here? Might need to enhance > that function to allow ne […] Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/10b9fbdb_8c9a2ae0 : PS3, Line 7029: VERIFY_PERMISSION > VERIFY_PERMISSION should go first Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/b4a5b082_b11cfef8 : PS3, Line 7037: if (cidr && !no_more_than_n_args(msglevel, p, 4, NM_QUOTE_HINT)) > Why NM_QUOTE_HINT? Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/4c4dea4c_37ac9b94 : PS3, Line 7042: OPT_P_ROUTE > VERIFY_PERMISSION should go first Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/23a2b49a_8cb1624d : PS3, Line 7047: const char *network = strtok(p[i++], "/"); /* this modifies p[1] */ > I find this very ugly. […] Yes, definitely. http://gerrit.openvpn.net/c/openvpn/+/739/comment/1f3bc400_01968578 : PS3, Line 7071: add_route_to_option_list(options->routes, p[1], NULL, p[2], p[3]); > Should we consider already normalizing the route here already and not parse > the netmask vs netbits l […] The parameters are all stored in string format because "network" and "gateway" can be ip addresses, resolvable names or even special keywords and the parsing is done in init_route(). If we were to search for the netbits here (in order to convert them into a netmask or vice versa) we would end up moving some of the init_route() logic here. Anyway, once ip_or_dns_addr_safe() is improved to handle CIDR notated addresses, do you still think the overhead introduced here is too much? http://gerrit.openvpn.net/c/openvpn/+/739/comment/03dc6477_3baf4f59 : PS3, Line 7409: if (!no_more_than_n_args(msglevel, p, 3, NM_QUOTE_HINT)) > Why NM_QUOTE_HINT? Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/c67b2950_32857d05 : PS3, Line 7424: msg(M_USAGE, "--server directive network/netmask combination is invalid"); > Why does this have a separate error message here but not in the sameish code > in --server-bridge belo […] Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/68375742_e67a2168 : PS3, Line 7487: if (!no_more_than_n_args(msglevel, p, 4, NM_QUOTE_HINT)) > Why NM_QUOTE_HINT? Which argument could contain spaces? None in fact, I just totally overlooked this flags honestly. Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/600e6ed7_2f6ddc3f : PS3, Line 7898: if (!no_more_than_n_args(msglevel, p, 3, NM_QUOTE_HINT)) > Why NM_QUOTE_HINT? Acknowledged File src/openvpn/route.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/ac8b37af_6f6e9a74 : PS3, Line 324: const in_addr_t default_netbits = IPV4_NETBITS_HOST; > unsigned int? Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/ee1f7533_80da73d9 : PS3, Line 331: const char *cidr = strchr(network, '/'); > Would convert to bool here to make it clear purpose Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/1b593f21_ba11a0bc : PS3, Line 347: convert > Want to do a drive-by fix? "a a" -> "to an"? Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/fcdc0ae1_bb844192 : PS3, Line 1105: route > Maybe add a 0.0.0.0/1 and 128.0.0. […] Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/3eb2716e_74185bc0 : PS3, Line 1166: route ( > Same (0.0.0.0/1 and 128.0.0.0/1) here. Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/2e313668_5df0fac8 : PS3, Line 1456: > Maybe we could move this into the if block? Not sure why we would create it > otherwise? Acknowledged File src/openvpn/tun.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/6a23e054_db476467 : PS3, Line 512: const bool looks_like_netmask = addr && ((addr & 0xFF000000) == 0xFF000000); > Isn't this redundant? If addr is 0 the second part never matches anyway Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/fe64f694_ae441e6d : PS3, Line 524: > Shouldn't that be "&&" ? Yes, indeed http://gerrit.openvpn.net/c/openvpn/+/739/comment/a2797ec0_d79ee4f7 : PS3, Line 562: const in_addr_t public_net = public &netmask; > Add space after & ? Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/7d21b2d6_5f6efde8 : PS3, Line 591: & > Add space after & Acknowledged -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/739?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: Iae04ad8715e40dfc76475c2c5b9a766c9604efc9 Gerrit-Change-Number: 739 Gerrit-PatchSet: 3 Gerrit-Owner: ralf_lici <r...@mandelbit.com> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Comment-Date: Mon, 16 Sep 2024 15:54:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@lichtenheld.com> Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
