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 19: Code-Review-2 (10 comments) Patchset: PS19: more comments, but these are mostly minor and/or nothing new. The main thing that must be fixed remains the gremlin crash... File src/openvpn/multi.h: http://gerrit.openvpn.net/c/openvpn/+/764/comment/acd4850a_3fd5d5d9 : PS19, Line 256: * on the transport protocols: The wording is a bit odd... since we do no longer need to point out that this calls "tcp and udp!!" I'd just make this ``` * Main event loop for OpenVPN in server mode ``` (yes it will call `tunnel_server_loop()` but that's not really relevant here, is it?) File src/openvpn/multi.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/002a17a2_4cb6329f : PS18, Line 609: bool is_dgram = proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto); > It is, the 'mi' tells you that this is a client-instance context. Done File src/openvpn/multi_io.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/4c1453d0_3cacda0f : PS19, Line 145: !proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto) ? &mi->tcp_rwflags : NULL); I like that this is being moved to a dedicated function :-) - I would style it differently, though, not "one fat function call with two conditionals in it". Maybe start with `if (!mi) { return; }` (early return, save indent space) and then have a surrounding `if(proto_is_dgram) { ...}` with one `socket_set()` call in each branch? ... also, why are we not storing `ev_arg` in the `&mi` for both cases? http://gerrit.openvpn.net/c/openvpn/+/764/comment/5d6f18e7_d50503ba : PS19, Line 491: } since this is the same `if (mi)` now, it could go outside one level, avoid code duplication... and make the comment "protocol invariant" (`/* monitor and/or handle events that are triggered...` without mentioning `TCP`) File src/openvpn/openvpn.h: http://gerrit.openvpn.net/c/openvpn/+/764/comment/ce61719a_f2258f18 : PS19, Line 219: * tunnel_point_to_point(), \c tunnel_server() \c an `and` is missing before `tunnel_server()` (since it's only these 2 functions now) File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/3dbf2e3f_8940878c : PS18, Line 3351: if (!le->proto) > Acknowledged it says `acknowledged` but we still have no comment for `proto`? ;-) http://gerrit.openvpn.net/c/openvpn/+/764/comment/ad01e4f2_8599b78a : PS18, Line 3839: > Acknowledged so what does "Acknowledged" mean, here? "Will be handled in a future patch"? File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/873691f9_0977c2fe : PS19, Line 1747: o->local_list->array[i]->port); I think this should show `proto` now as well, otherwise the log is a bit hard to read if trying to understand why a `bind()` fails. File src/openvpn/socket.c: http://gerrit.openvpn.net/c/openvpn/+/764/comment/e7e767ef_7f49bef8 : PS18, Line 1914: ASSERT(c->c2.link_sockets[sock_index]); > Done maybe gerrit is playing tricks on me, but the `ASSERT()` seems to be still here -- 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: 19 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: Tue, 18 Feb 2025 17:35:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: its_Giaan <gianma...@mandelbit.com> Comment-In-Reply-To: cron2 <g...@greenie.muc.de> Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
