Attention is currently required from: its_Giaan, ordex, plaisthos. cron2 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: Code-Review-1 (23 comments) Patchset: PS18: some minor bits, and something I consider major - massive code duplication that is not helping to make the code flow easier to understand File doc/man-sections/link-options.rst: http://gerrit.openvpn.net/c/openvpn/+/764/comment/37a21a77_b85f0053 : PS18, Line 122: I think we also should mention the default values, something like ``` If ``port`` is omitted, it defaults to the setting of ``--lport``. If ``proto`` is omitted, it defaults to the setting of ``--proto``. ``` ... which brings up an interesting corner case, what if a config has ``` local * 1000 tcp remote 1.2.3.4 1194 udp ``` ? Shall we just disallow `proto` bindings on the client-side? File src/openvpn/forward.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/6695b917_b3e1ecda : 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, without appearing twice. Can you remove the common parts from `io_wait_dowork()` and have it call here? http://gerrit.openvpn.net/c/openvpn/+/764/comment/f3a3f9ad_e4a8f66f : PS18, Line 2184: if (c->c2.fast_io && (flags & (IOW_TO_TUN | IOW_TO_LINK | IOW_MBUF))) this function is mostly identical to `forward.h::io_wait()`, except for the call to `get_io_flags_dowork_udp()` in the end, vs. `io_wait_dowork()`, and setting `multi_io->udp_flags` vs. `c->c2.event_set_status`. There has to be a better way to tackle this than to walk all sockets twice, in two nearly identical functions. Why's there a separate `udp_flags` and `event_set_status` anyway? File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/bf3f0236_95dbba2c : PS17, Line 2846: if (has_udp_in_local_list(&c->options)) > maybe something to create a ticket about and we fix it separately from this > patchset? […] Well, this is introducing new code, to model behaviour that was not intentional in the first place. So I'd go for simple code - on a server, no matter of UDP or TCP, delay 1 second, on a client, do exponential backoff. Also, arguably, the code proposed isn't doing the right thing anyway - it's doing "if there is *one* UDP socket, among many, do backoff (on UDP and TCP sockets), and if there are *only* TCP sockets, not". File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/286fdebf_e60bf789 : 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... I was wondering, again, why there is no loop here... File src/openvpn/mtcp.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/ed3a23f0_5a81d266 : PS17, Line 57: if (mi && !proto_is_dgram(ls->info.proto)) > that's a leftover from early development when still trying to untangle udp > and tcp clients. Acknowledged http://gerrit.openvpn.net/c/openvpn/+/764/comment/7810367d_c39b0d92 : PS17, Line 156: if (proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto)) > see answer above Acknowledged File src/openvpn/mudp.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/e0aa2662_4a802ef8 : PS18, Line 335: const unsigned int status = m->multi_io->udp_flags; I do wonder why a separate `udp_flags` field for UDP has been introduced. Isn't this basically mimicking everything we do in `event_set_status` for TCP? Where is the practical difference (except we have two mostly-identical functions that set udp_flags or event_set_status)? File src/openvpn/multi.h: http://gerrit.openvpn.net/c/openvpn/+/764/comment/b5407429_3ff1a65d : PS18, Line 670: how can this ever be called without valid `mi`? Shouldn't this be flagged as a hard error, like keeping the `ASSERT(mi)`? File src/openvpn/multi.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/c2b835ed_a70ba824 : 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 only look at `link_sockets[0]`... ;-) http://gerrit.openvpn.net/c/openvpn/+/764/comment/35e37e61_90cb74af : PS18, Line 3841: if (!mi->halt && proto_is_dgram(mi->context.options.ce.proto)) why is this using `mi->context.options.ce.proto`? The rest of the code seems to have moved all the checks toward something `ls->proto` - is this not available here? http://gerrit.openvpn.net/c/openvpn/+/764/comment/9539d5ff_a442fabf : 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()` to `has_udp_...()` but it could just be part of the same `if (...&...&...)` block. no? http://gerrit.openvpn.net/c/openvpn/+/764/comment/82416e50_ccc42bb0 : 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 server mode` instead, so it's clear "this is the --server functionality". http://gerrit.openvpn.net/c/openvpn/+/764/comment/0808d8b9_ee6cdc19 : PS18, Line 4169: void static? http://gerrit.openvpn.net/c/openvpn/+/764/comment/9c492ef1_466fc2d6 : PS18, Line 4265: } I don't like this construct - now `tunnel_server_init()` is doing all the work (init, call loop, uninit). I suggest to remove `tunnel_server_init()` and have all its functionality in `tunnel_server()`. Having the inner loop in `tunnel_server_loop()` is fine for me. File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/3fed854f_635a4c70 : PS10, Line 1000: for (int j = 0; j < e->local_list->len; j++) > made another function to do this specifically for every local_entry, let me > know if this is legal. Acknowledged http://gerrit.openvpn.net/c/openvpn/+/764/comment/bed3e7ba_f462f4f9 : PS10, Line 3833: if (!has_udp_in_local_list(o)) > made another function has_tcp_in_local_list(), might be useful in some cases, > also I moved this chec […] Acknowledged File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/b093c46d_db331cc3 : PS17, Line 1052: setenv_local_entry(es, o->ce.local_list->array[i], i+1); > i'm not sure either so only one "i+1" left - that means local_list entries are numbered 0...(n-1) and the env variables are numbered 1...n - which makes sense. File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/231e4ca1_8cdb4f3f : PS18, Line 3351: if (!le->proto) we have a comment for setting "port" and none for "proto". Do we want one? http://gerrit.openvpn.net/c/openvpn/+/764/comment/ffe64598_39b97862 : PS18, Line 3816: } this extra loop looks like it really wants to be part of `options_postprocess_mutate_le()`, no? http://gerrit.openvpn.net/c/openvpn/+/764/comment/da8501e5_f0ca4bac : PS18, Line 3839: I'm actually not sure I understand `fast_io` well enough for this, but - what I understand, `fast_io` is only working on UDP sockets (and not if `--shaper` is active) - but why can't we not have `fast_io` on "all the UDP sockets" just because there is one extra TCP socket? File src/openvpn/socket.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/2d46cfa3_40e4c922 : PS18, Line 1914: ASSERT(c->c2.link_sockets[sock_index]); we already have an `ASSERT(sock)` at the beginning, which is `sock = c->c2.link_sockets[sock_index];` - so I think this can go? -- 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: its_Giaan <gianma...@mandelbit.com> Gerrit-Attention: ordex <a...@unstable.cc> Gerrit-Comment-Date: Fri, 31 Jan 2025 13:32:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: cron2 <g...@greenie.muc.de> Comment-In-Reply-To: its_Giaan <gianma...@mandelbit.com> 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
