Attention is currently required from: its_Giaan, ordex, plaisthos.
cron2 has posted comments on this change. (
http://gerrit.openvpn.net/c/openvpn/+/434?usp=email )
Change subject: Adapt socket handling to support listening on multiple sockets
......................................................................
Patch Set 7: Code-Review-2
(17 comments)
Patchset:
PS7:
Okay, this is a big one.
There is minor things - whitespace changes that should not be done, or wrapping
changes that are not called for. There are some parts that I just do not
understand which would benefit from comments in the code *or* comments here in
gerrit ("will be resolved by the next patch in the series").
There's ugliness (link_socket_init_phase1(), the patch looks excessive for what
it intends to do).
There are at least 3 places that look like rebase accidents, and need to be
very carefully checked for "is this change intentional".
File src/openvpn/forward.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/af521d3c_72b59ce8 :
PS7, Line 1133: /* all sockets are of the same type, so just check
the first one */
this confuses me. We will have UDP and TCP sockets later on, so they are not
expected to be "all of the same type". Will this be addressed later in the
patch series?
File src/openvpn/init.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/e3b56022_c021a1c0 :
PS7, Line 533: {
this is a behavioural change, not checking `advance_next_remote` anymore. Is
this intentional or a rebase artefact?
http://gerrit.openvpn.net/c/openvpn/+/434/comment/d9600958_55933535 :
PS7, Line 1959: ASSERT(c->c2.link_sockets);
this is not doing the same thing as before... so we do not know if
`link_sockets[0]` is actually well defined here. No?
http://gerrit.openvpn.net/c/openvpn/+/434/comment/f4f59153_2fd3ff1d :
PS7, Line 2015: do_ifconfig(c->c1.tuntap,
c->c1.tuntap->actual_name, c->c2.frame.tun_mtu, c->c2.es, &c->net_ctx);
line length exceeded, and the old version wasn't that bad (the unwrapping above
was reasonable, with just a few characters hanging in the next line, but this
one should stay as it is).
Also, quite unrelated to the patch itself... just sayin.
http://gerrit.openvpn.net/c/openvpn/+/434/comment/2d3860a3_9d493c40 :
PS7, Line 3819: do_init_socket_1(struct context *c)
I think we should stick to `phase1` as function name, otherwise it reads as if
this is "init socket 1", and "init socket 2" for "socket #1 and #2". No?
http://gerrit.openvpn.net/c/openvpn/+/434/comment/f10b10f2_22a5be55 :
PS7, Line 4011: if (c->c2.link_sockets && c->c2.link_socket_owned)
what happened to the `is_dco_win()` part here? This looks like a rebase
accident
http://gerrit.openvpn.net/c/openvpn/+/434/comment/72fd22c0_b99b4657 :
PS7, Line 4040: CLEAR(c->c1.link_socket_addrs[0].actual);
this looks inconsistent - freeaddrinfo() is called in the loop, while the
CLEAR() is only called for [0]. Intentional? If yes, a comment should say so.
(I could see this as being intentional "because this is client side only, and
client only has 1 socket, ever", but it looks weird)
http://gerrit.openvpn.net/c/openvpn/+/434/comment/b96d510f_3ce9a525 :
PS7, Line 4719: do_init_socket_1(c);
mmmh, see comment above, maybe name this `do_init_link_sockets_phase1()`?
`do_init_socket_1()` could be anything...
http://gerrit.openvpn.net/c/openvpn/+/434/comment/e0ffc177_bec7efd2 :
PS7, Line 4762: do_init_socket_2(c);
`_phase2()`
http://gerrit.openvpn.net/c/openvpn/+/434/comment/1785554f_4c253f1b :
PS7, Line 4968: &dest->gc);
is this wrapping necessary? Comparing to the lines below it does not seem like
it
File src/openvpn/mudp.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/ed1d30f4_2e4e41d3 :
PS7, Line 384: for (int i = 0; i < m->top.c1.link_sockets_num; i++)
this, again, is confusing me. So is this a client instance ("1 socket exists")
or the server instance ("all sockets exists, but sockets can be UDP and TCP")?
Having the `_udp()` handler walk through the list of sockets which could be
"more than 1, and such, UDP or TCP" does not look right, at least not without
some explanation ("this will all move to a common `multi_process_io_anything()`
in the next patch") ;-)
File src/openvpn/socket.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/efd059f1_5a25f7ad :
PS7, Line 1858: c->c2.link_sockets[sock_index]->local_host =
c->options.ce.local;
Can this be changed to `*sock = &c->c2.link_sockets[sock_index]` and then the
rest of the function can be left unchanged? this is how all the other parts of
the patch do it... (not `[0]` in this case, of course).
http://gerrit.openvpn.net/c/openvpn/+/434/comment/b3f1e813_3ace2227 :
PS7, Line 2222: #if defined (_WIN32)
unrelated whitespace change, please do not (also, this extra space is not how
it's done elsewhere)
http://gerrit.openvpn.net/c/openvpn/+/434/comment/2e93e0ee_51a6abd7 :
PS7, Line 2259: sig_info->signal_received = SIGUSR1;
I think this is a rebase artefact, the multisocket patch colliding with Selva's
signal handler reworking. Please double-check, but this does not look right.
http://gerrit.openvpn.net/c/openvpn/+/434/comment/fa7b3095_61edff01 :
PS7, Line 2271: sig_info);
please do not wrap. The line is not long and not changed by this patch.
http://gerrit.openvpn.net/c/openvpn/+/434/comment/02a03a9a_2d541041 :
PS7, Line 2297: if (sig_save)
again, please thoroughly double check. I think this is old code, and the
throw_signal() stuff is how it should be.
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/434?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: Ia0a889e800f0b36aed770ee36e31afeec5df6084
Gerrit-Change-Number: 434
Gerrit-PatchSet: 7
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: its_Giaan <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: its_Giaan <[email protected]>
Gerrit-Attention: ordex <[email protected]>
Gerrit-Comment-Date: Wed, 27 Nov 2024 15:54:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
