Attention is currently required from: plaisthos, ralf_lici. flichtenheld 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: Code-Review-2 (16 comments) Patchset: PS3: Didn't get through the whole code, but here a collection of my thoughts so far... File doc/man-sections/server-options.rst: http://gerrit.openvpn.net/c/openvpn/+/739/comment/04a8784d_9d0d84d0 : PS3, Line 344: :code:`255.255.255.255`. for completeness mentions /32 as well. File doc/man-sections/vpn-network-options.rst: http://gerrit.openvpn.net/c/openvpn/+/739/comment/7f755556_b75a572a : PS3, Line 220: ifconfig local rn Need to specify the two possibilities (IP address or netmask) separately. Also "rn" is a horrible name. What it is supposed to mean? http://gerrit.openvpn.net/c/openvpn/+/739/comment/9e59c2d1_8882b7e2 : PS3, Line 266: 2 255 Add /bits example here? http://gerrit.openvpn.net/c/openvpn/+/739/comment/f33e5bdd_a494b3ca : 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 --route-ipv6 ? File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/f3ac4520_d5a41ab2 : PS2, Line 1535: msg(D_SHOW_PARMS, " ifconfig_pool_start = %s/%d", print_in_addr_t(o->ifconfig_pool_start, 0, &gc), o->ifconfig_pool_netbits); > ok Done File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/483600ca_2e578b22 : 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 netbits, though. http://gerrit.openvpn.net/c/openvpn/+/739/comment/37d80b88_d3071bd7 : PS3, Line 7029: VERIFY_PERMISSION VERIFY_PERMISSION should go first http://gerrit.openvpn.net/c/openvpn/+/739/comment/babf9754_f9b70e73 : PS3, Line 7037: if (cidr && !no_more_than_n_args(msglevel, p, 4, NM_QUOTE_HINT)) Why NM_QUOTE_HINT? http://gerrit.openvpn.net/c/openvpn/+/739/comment/e96e9d33_aa4c39e3 : PS3, Line 7042: OPT_P_ROUTE VERIFY_PERMISSION should go first http://gerrit.openvpn.net/c/openvpn/+/739/comment/1c383a62_67435f56 : PS3, Line 7047: const char *network = strtok(p[i++], "/"); /* this modifies p[1] */ I find this very ugly. Would be better if we could improve ip_or_dns_addr_safe to handle CIDR notation, wouldn't it? http://gerrit.openvpn.net/c/openvpn/+/739/comment/4bb91f0f_a5b37ba7 : 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 later _again_? But I have to confess I'm not sure why we store the strings here in the first place and do not parse them already, so I might be missing something. http://gerrit.openvpn.net/c/openvpn/+/739/comment/70b69c9f_0e173b5d : PS3, Line 7409: if (!no_more_than_n_args(msglevel, p, 3, NM_QUOTE_HINT)) Why NM_QUOTE_HINT? http://gerrit.openvpn.net/c/openvpn/+/739/comment/eed071e7_4e1a613b : 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 below? Can probably remove it here. http://gerrit.openvpn.net/c/openvpn/+/739/comment/ef3a007f_b7b2f353 : PS3, Line 7487: if (!no_more_than_n_args(msglevel, p, 4, NM_QUOTE_HINT)) Why NM_QUOTE_HINT? Which argument could contain spaces? http://gerrit.openvpn.net/c/openvpn/+/739/comment/5a041385_78596de3 : PS3, Line 7898: if (!no_more_than_n_args(msglevel, p, 3, NM_QUOTE_HINT)) Why NM_QUOTE_HINT? -- 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: ralf_lici <r...@mandelbit.com> Gerrit-Comment-Date: Fri, 13 Sep 2024 13:57:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: flichtenheld <fr...@lichtenheld.com> Comment-In-Reply-To: ralf_lici <r...@mandelbit.com> Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
