Attention is currently required from: cron2, srkyn. razvanc has posted comments on this change by srkyn. ( http://gerrit.openvpn.net/c/openvpn/+/1881?usp=email )
Change subject: compat: close all unrelated fds in forked helpers ...................................................................... Patch Set 4: (15 comments) File CMakeLists.txt: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/539ea40c_e5e98995?usp=email : PS2, Line 699: ) > This makes it look as if test_close_fds is somehow related to test_cryptoapi. > […] Done http://gerrit.openvpn.net/c/openvpn/+/1881/comment/7462133d_279dc489?usp=email : PS2, Line 852: tests/unit_tests/openvpn/mock_get_random.c > Do you need `mock_get_random.c` here? The Automake version doesn't add it and > it seems fine. Done File src/compat/close-fds.h: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/6dbe1f11_567fd97a?usp=email : PS1, Line 13: */ > The rest of the files have a more comprehensive block of text about the > license, we probably want to […] Done http://gerrit.openvpn.net/c/openvpn/+/1881/comment/4023b0a5_04fb73fc?usp=email : PS1, Line 19: void close_fds_except(int keep); > The new function lost all previous (Doxygen) documentation explaining the > need for it and what it do […] Done File src/compat/close-fds.c: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/0f8f44d9_be569440?usp=email : PS1, Line 12: * as published by the Free Software Foundation. > Truncated license text compared to existing files here as well. Done File src/compat/close-fds.c: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/1501e466_54f5b66c?usp=email : PS2, Line 39: #ifdef HAVE_CLOSE_RANGE > I could be wrong (maybe I missed something subtle), but AFAICT this path can > never be exercised unde […] Done http://gerrit.openvpn.net/c/openvpn/+/1881/comment/0c59f966_7025f8c8?usp=email : PS2, Line 42: const int lower_closed = keep == 3 || close_range(3, (unsigned int)keep - 1, 0) == 0; > ... and please avoid complex conditional operator constructs with side > effects in assignments. […] Done File src/compat/close-fds.c: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/1056f83b_1857d0a4?usp=email : PS3, Line 1: /* > It looks like all the other files in src/compat have a naming convention of > compat-*. […] Done http://gerrit.openvpn.net/c/openvpn/+/1881/comment/d89b0a25_26c6a7bb?usp=email : PS3, Line 64: } > This can probably be done in a synonymous but shorter form, i.e. […] Done File tests/unit_tests/openvpn/Makefile.am: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/05fbb928_b6b56f7e?usp=email : PS2, Line 52: close_fds_testdriver_CFLAGS = -I$(top_srcdir)/src/compat @TEST_CFLAGS@ > This is the only testdriver whose CFLAGS omit `-I$(top_srcdir)/src/openvpn`. > […] Done File tests/unit_tests/openvpn/test_close_fds.c: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/fd8640c3_ab378458?usp=email : PS1, Line 12: * as published by the Free Software Foundation. > License header doesn't match the rest of the code in length. Done http://gerrit.openvpn.net/c/openvpn/+/1881/comment/1bf78c4e_5f1d8fce?usp=email : PS1, Line 36: run_case(int keep_target) > The current convention AFAICT is to use cmocka for tests, which this does not. Done http://gerrit.openvpn.net/c/openvpn/+/1881/comment/6706f88e_1549c3e0?usp=email : PS1, Line 109: : EXIT_FAILURE; > Might we also want to test that fds 0, 1 and 2 survive? Done File tests/unit_tests/openvpn/test_misc.c: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/95ace02a_fef92240?usp=email : PS3, Line 544: if (dup2(source, 100) < 0 || dup2(source, 200) < 0) > The new tests fail when `RLIMIT_NOFILE` ≤ 200 (e.g. `ulimit -n 64` causes > them to fail. […] Done File tests/unit_tests/openvpn/test_misc.c: http://gerrit.openvpn.net/c/openvpn/+/1881/comment/c80f52f3_43159fc8?usp=email : PS4, Line 567: : '0'; This has the tests pass even without the patch, since `dup(source)` will return the lowest free descriptor, so these should always be <= 6 here. Ideally a unit test should fail without the fix and pass once the fix lands. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1881?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: Iacd56d245e9ab30cfb25b2b364ca661fa289dc55 Gerrit-Change-Number: 1881 Gerrit-PatchSet: 4 Gerrit-Owner: srkyn <[email protected]> Gerrit-Reviewer: plaisthos <[email protected]> Gerrit-Reviewer: razvanc <[email protected]> Gerrit-CC: cron2 <[email protected]> Gerrit-CC: openvpn-devel <[email protected]> Gerrit-Attention: cron2 <[email protected]> Gerrit-Attention: srkyn <[email protected]> Gerrit-Comment-Date: Mon, 07 Sep 2026 09:56:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <[email protected]> Comment-In-Reply-To: razvanc <[email protected]>
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
