Attention is currently required from: flichtenheld, plaisthos. ralf_lici has posted comments on this change by ralf_lici. ( http://gerrit.openvpn.net/c/openvpn/+/739?usp=email )
Change subject: options: add IPv4 CIDR parsing for relevant directives ...................................................................... Patch Set 9: (5 comments) File doc/man-sections/server-options.rst: http://gerrit.openvpn.net/c/openvpn/+/739/comment/93ccbb1c_e6347e78?usp=email : PS9, Line 309: view. > I would find it a bit easier to read if this was more explicit. […] Acknowledged File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/bc6ae7da_894ede5f?usp=email : PS9, Line 7288: const int lev = M_WARN; > Please fix to msglvl_t Acknowledged http://gerrit.openvpn.net/c/openvpn/+/739/comment/25bbf8a6_50aad5ce?usp=email : PS9, Line 7688: remote_netmask = getaddr(GETADDR_HOST_ORDER | GETADDR_RESOLVE, ifconfig_push_parms[2], 0, > A bit absurd that we need to parse netmask here again if we generated it > before. […] I agree. The main issue is that IPv4 options in options.c do not all convert at the same stage: some paths keep parameters as strings until later consumers, while others parse to binary earlier. A dual-helper design (string + binary outputs) is possible, but it would either duplicate per-option handling or require a broader parsing abstraction. For this change, the scope is limited to translating CIDR-form IPv4 parameter strings into the existing legacy string form, without altering downstream option processing. File src/openvpn/options_util.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/eb44c41a_68747e58?usp=email : PS9, Line 232: * socket_util.c dependencies into options_util unit-test targets. */ > This feels like a weak justification to duplicate code. Just to make sure I understood this point correctly: do you prefer that I switch to print_in_addr_t() (and pull in socket_util dependencies), or that I keep the local conversion and only tighten/remove the justification comment? I tried the former locally: switching to print_in_addr_t() made options_parse_testdriver, push_update_msg_testdriver, and misc_testdriver fail to link unless socket_util.c is also linked. Once socket_util.c is linked, additional transitive symbols are required (setenv_*, siginfo_static/signal_reset, management_*), so this becomes a broader test/build refactor. That's why for this patch I kept the conversion self-contained in options_util.c. File tests/unit_tests/openvpn/test_options_parse.c: http://gerrit.openvpn.net/c/openvpn/+/739/comment/306d8e15_e3a09146?usp=email : PS9, Line 330: test_convert_ipv4_cidr_parms_coverage(void **state) > This does not feel like it provides actual "coverage". The examples are all > too similar. […] 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?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Iae04ad8715e40dfc76475c2c5b9a766c9604efc9 Gerrit-Change-Number: 739 Gerrit-PatchSet: 9 Gerrit-Owner: ralf_lici <[email protected]> Gerrit-Reviewer: flichtenheld <[email protected]> Gerrit-Reviewer: plaisthos <[email protected]> Gerrit-CC: openvpn-devel <[email protected]> Gerrit-Attention: plaisthos <[email protected]> Gerrit-Attention: flichtenheld <[email protected]> Gerrit-Comment-Date: Tue, 10 Mar 2026 09:53:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <[email protected]>
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
