Attention is currently required from: cron2, ordex, plaisthos. its_Giaan has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/764?usp=email )
Change subject: Add support for simultaneous use of UDP and TCP sockets ...................................................................... Patch Set 18: (15 comments) File src/openvpn/forward.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/50e5e827_606d6e4d : PS18, Line 2077: if (flags & IOW_WAIT_SIGNAL) > this is duplicating a huge chunk of code from `io_wait_dowork()` - which is > hard enough to read, wit […] Done http://gerrit.openvpn.net/c/openvpn/+/764/comment/d0b6ff1d_733c10fb : PS18, Line 2184: if (c->c2.fast_io && (flags & (IOW_TO_TUN | IOW_TO_LINK | IOW_MBUF))) > this function is mostly identical to `forward. […] The multi_io has no event_set_status member and since UDP require that I decided to call it udp_flags since it is UDP specific. File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/f536d12b_7a5c51e7 : PS17, Line 2846: if (has_udp_in_local_list(&c->options)) > Well, this is introducing new code, to model behaviour that was not > intentional in the first place. […] Done File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/29430a8c_d3861afa : PS18, Line 2685: if (!proto_is_udp(c->c2.link_sockets[0]->info.proto) > maybe add a de-confusing comment like `/* client side = only one link_socket > */` or so... […] Done File src/openvpn/multi.h: http://gerrit.openvpn.net/c/openvpn/+/764/comment/cad1ca81_6565d700 : PS18, Line 670: > how can this ever be called without valid `mi`? Shouldn't this be flagged as > a hard error, like kee […] Done File src/openvpn/multi.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/6aa7274d_985a29c2 : PS18, Line 609: bool is_dgram = proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto); > Is this for a (single-socket) client instance? Again we have a confused > reader wondering why we onl […] It is, the 'mi' tells you that this is a client-instance context. http://gerrit.openvpn.net/c/openvpn/+/764/comment/61d7a697_54c1c90d : PS18, Line 3841: if (!mi->halt && proto_is_dgram(mi->context.options.ce.proto)) > why is this using `mi->context.options.ce. […] Done http://gerrit.openvpn.net/c/openvpn/+/764/comment/02b15a25_8acde395 : PS18, Line 3879: else if (has_udp_in_local_list(&m->top.options)) > why are you splitting this condition into two nested `if()`? I can see > changing `proto_is_dgram()` […] Done http://gerrit.openvpn.net/c/openvpn/+/764/comment/0f048603_319d08a4 : PS18, Line 4164: * Main event loop for OpenVPN in multi-protocol server mode. > I'd remove the "multi-protocol" part (protocols, again) - maybe name this > `point-to-multipoint serve […] Done http://gerrit.openvpn.net/c/openvpn/+/764/comment/20464f08_18d9e039 : PS18, Line 4169: void > static? Done http://gerrit.openvpn.net/c/openvpn/+/764/comment/d0311291_8207b195 : PS18, Line 4265: } > I don't like this construct - now `tunnel_server_init()` is doing all the > work (init, call loop, uni […] Done File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/cef8e785_9aa6185f : PS18, Line 3351: if (!le->proto) > we have a comment for setting "port" and none for "proto". […] Acknowledged http://gerrit.openvpn.net/c/openvpn/+/764/comment/c5c0f206_666146e4 : PS18, Line 3816: } > this extra loop looks like it really wants to be part of > `options_postprocess_mutate_le()`, no? Acknowledged http://gerrit.openvpn.net/c/openvpn/+/764/comment/611fa1ff_cab4dcb3 : PS18, Line 3839: > I'm actually not sure I understand `fast_io` well enough for this, but - what > I understand, `fast_io […] Acknowledged File src/openvpn/socket.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/68eeaf4b_34101cc0 : PS18, Line 1914: ASSERT(c->c2.link_sockets[sock_index]); > we already have an `ASSERT(sock)` at the beginning, which is `sock = c->c2. > […] Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/764?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: I31bbf87e4e568021445c7512ecefadfd4a69b363 Gerrit-Change-Number: 764 Gerrit-PatchSet: 18 Gerrit-Owner: its_Giaan <gianma...@mandelbit.com> Gerrit-Reviewer: cron2 <g...@greenie.muc.de> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-CC: ordex <a...@unstable.cc> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: cron2 <g...@greenie.muc.de> Gerrit-Attention: ordex <a...@unstable.cc> Gerrit-Comment-Date: Fri, 14 Feb 2025 17:45:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <g...@greenie.muc.de> Comment-In-Reply-To: ordex <a...@unstable.cc> Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
