Attention is currently required from: flichtenheld, plaisthos, stipa. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/815?usp=email )
Change subject: dco-win: multipeer support ...................................................................... Patch Set 18: Code-Review-1 (13 comments) Patchset: PS18: Oh well, when I start, I tend to find things... sorry for that. File src/openvpn/dco_win.h: http://gerrit.openvpn.net/c/openvpn/+/815/comment/8bc501cb_073ac40b : PS18, Line 42: dco_mode_type mode; I'm not really happy with that. `dco_linux` has its own very similar enum, with uppercase `DCO_MODE_<xx>` and calls the member variable `dco->ifmode`. Since it does the same thing, can you name the windows variant `ifmode` as well, and also use uppercase for the enum? (re-using the enum is not trivial because it comes from `ovpn_dco_linux.h` but won't be necessary, I think, as long as it's clear "it does the same thing"). Maybe `ovpn_dco_init()` can also be modeled to be more lookalike? File src/openvpn/dco_win.c: http://gerrit.openvpn.net/c/openvpn/+/815/comment/9f12680a_836a673a : PS18, Line 127: A few words of comment somewhere on the call chain "who calls what, in which sequence" for p2p and mp mode would help me get less confused... there's so many "open", "init" and "start" functions here. http://gerrit.openvpn.net/c/openvpn/+/815/comment/410773f9_bc0b637c : PS18, Line 132: dco->mode = (mode == MODE_POINT_TO_POINT) ? dco_mode_p2p : dco_mode_mp; that comment is slightly confusing - if it is "already set", then why do we set it again? http://gerrit.openvpn.net/c/openvpn/+/815/comment/f14e44de_56a2dc7b : PS18, Line 146: OVPN_MODE m = OVPN_MODE_MP; here we have a new define for communication between driver and userland, which does the same as `dco_mode_mp`... http://gerrit.openvpn.net/c/openvpn/+/815/comment/aaa33b91_1d10db5c : PS18, Line 291: if (out.ListenAddress.Addr4.sin_family == AF_INET) This part confuses me. What is `out`? Why is it put into `actual.dest`? This seems to be the address of the peer we're talking to, judging from other uses of `actual.dest` but then it shouldn't be tied to the "global" DCO opening, but to individual peers? Or am I totally misunderstanding this? http://gerrit.openvpn.net/c/openvpn/+/815/comment/ac5dc11e_4884b3b4 : PS18, Line 393: { I do wonder why this is necessary. "Use best source for destination" is what happens automatically when passing an UDP packet without source to `sendto()`. Is this different in kernel space? Why was this not needed for p2p win-dco? (As in, if kernel space will automatically put the "best source" in the packet anyway, we can just get rid of all code dealing with "best_src") File src/openvpn/forward.c: http://gerrit.openvpn.net/c/openvpn/+/815/comment/c2131d8c_f342751d : PS18, Line 1326: bool prepend_sa = c->options.mode == MODE_SERVER && dco_enabled(&c->options); we do have `SF_PREPEND_SA` set on the sockflags - is that available here? File src/openvpn/ovpn_dco_win.h: http://gerrit.openvpn.net/c/openvpn/+/815/comment/bd9f5732_254ace41 : PS18, Line 120: } OVPN_CRYPTO_DATA_V2, * POVPN_CRYPTO_DATA_V2; these two are not part of *this* patch set, and might need revisiting for "the new epoch thing" File src/openvpn/socket.c: http://gerrit.openvpn.net/c/openvpn/+/815/comment/141b9fd2_5cf1aa60 : PS18, Line 2256: else since there is a "goto done" in the `if()` we could get rid of the `else` and the dangling brackets outside of the #endif... http://gerrit.openvpn.net/c/openvpn/+/815/comment/0df307dd_57fedb84 : PS18, Line 3857: sa_len = sizeof(struct sockaddr_in); the repetition of `sizeof(struct sockaddr_in)` makes my eyes hurt... what about assigning `sa_len` first and then doing `if (buf_len() < sa_len)... memcpy(...sa_len)` etc? http://gerrit.openvpn.net/c/openvpn/+/815/comment/f9087006_6ac2ed8b : PS18, Line 3867: sa_len = sizeof(struct sockaddr_in6); same here http://gerrit.openvpn.net/c/openvpn/+/815/comment/445a211f_710bf77b : PS18, Line 3957: read_sockaddr_from_overlapped(io, &from->dest.addr.sa, ret); I do not understand these two `sh.is_handle` cases. Isn't this always a handle on windows? Or is this the "regular socket" case, and only wintun + dco have `is_handle`? Also, the conditon on `if (from)` is interesting, so if `from` happens to be NULL, we wouldn't `read_sockaddr_from_packet()` and subsequently the packet will be malformed... can it ever be NULL here? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/815?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: Ia267276d61fa1425ba205f54ba6eb89021f32dba Gerrit-Change-Number: 815 Gerrit-PatchSet: 18 Gerrit-Owner: stipa <lstipa...@gmail.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-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Attention: stipa <lstipa...@gmail.com> Gerrit-Comment-Date: Wed, 05 Feb 2025 19:11:20 +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
