Attention is currently required from: flichtenheld, its_Giaan, ordex, plaisthos.
cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/765?usp=email ) Change subject: mroute: adapt to new protocol handling and hashing improvements ...................................................................... Patch Set 14: Code-Review-2 (8 comments) Patchset: PS14: There is a couple of things that need looking at - the forward.c hunk should go to a patch in the series that actually deals with socket arrays, and also very very closely checked for correctness - the `proto` initialization in the maddr could be done in a more elegant way - `mroute_addr_hash_len()` needs to be fixed File src/openvpn/forward.c: http://gerrit.openvpn.net/c/openvpn/+/765/comment/3e5d8af2_a31c87b9 : PS14, Line 1132: for (int i = 0; i < c->c1.link_sockets_num; i++) I assume this hunk is needed, but it's sort of "outside the scope of mroute handling" (so should either move to #764 or be mentioned in the commit message - I think "moving" is more reasonable). Also I am not sure it is *correct* - if I read this correctly, the effect is now `if (!decrypt_status && we have *any* listening TCP socket) { SIGUSR1; }` no? As it's walking the whole array, not checking the socket where this packet was received. http://gerrit.openvpn.net/c/openvpn/+/765/comment/35afcb51_f6ae0b38 : PS14, Line 1135: /* on the instance context we have only one socket, so just check the first one */ whatever the outcome is, this comment is no longer correct :-) File src/openvpn/mroute.h: http://gerrit.openvpn.net/c/openvpn/+/765/comment/cad156ad_89f96d93 : PS14, Line 225: return (uint32_t) a->len + 2; if you change the start of the to-be-hashed block, but all the existing data should still be hashed, this looks like you also must increase the length by +1? File src/openvpn/mudp.c: http://gerrit.openvpn.net/c/openvpn/+/765/comment/7a2c3469_2b108655 : PS14, Line 196: m->local.proto = real.proto; This is a bit confusing. I can see the assignment to `real.proto` (because mroute_extract_openvpn_sockaddr() cannot do it) but what is `m->local`, and who is poulating the rest of it? File src/openvpn/multi.c: http://gerrit.openvpn.net/c/openvpn/+/765/comment/ff5809d5_e5708d45 : PS14, Line 1156: struct mroute_addr *addr, this looks wrong. A lookup function should not modify its arguments. I see calls to `multi_get_instance_by_virtual_addr()` being prepared by `mroute_extract_addr_from_packet()` calls - so if that function sets `proto = 0` (or just `CLEAR()`s the mroutes) we do not need to do anything here. http://gerrit.openvpn.net/c/openvpn/+/765/comment/8164b377_9ec4fcaa : PS14, Line 3356: see comment above - now you're actually setting `proto` in the caller, even if I do not agree this is the right place - so the change to `multi_get_instance_by_virtual_addr()` is definitely not needed. I'd still move this into `mroute_extract_addr_from_packet()`, with the comment. http://gerrit.openvpn.net/c/openvpn/+/765/comment/81dba5d2_42f25dc3 : PS14, Line 3561: dest.proto = src.proto; same thing -> `mroute_extract_addr_from_packet()` -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/765?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: Ic66eccb5058fe9c0fae64d8e2ca88728068a92ab Gerrit-Change-Number: 765 Gerrit-PatchSet: 14 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: flichtenheld <fr...@lichtenheld.com> Gerrit-Attention: ordex <a...@unstable.cc> Gerrit-Comment-Date: Fri, 24 Jan 2025 08:00:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
