Initial support for Multipath TCP on recent Linux kernels #1
Conversation
Signed-off-by: Arınç ÜNAL <[email protected]>
configure.ac
Outdated
| return(-1); | ||
| } | ||
| ]] | ||
| ) ], |
There was a problem hiding this comment.
I don't think this is needed: often these days, the binaries are compiled on a build host, probably with a different kernel that the one end-users will have.
In other words, I don't think you should check if the kernel support this at build time. It has to be done later.
What might be useful is to only define
ENABLE_MPTCPon Linux target. Then you don't need to have#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)in the code, justifdef ENABLE_MPTCP
There was a problem hiding this comment.
@matttbe sorry I've got no experience with autoconf. Do you mean something like this:
dnl
dnl Enable checking Multipath TCP support on Linux
dnl
case "$host" in
*-*-linux*)
AC_DEFINE(ENABLE_MPTCP, 1, [Enable checking Multipath TCP support])
;;
esac
There was a problem hiding this comment.
I don't think you should do that there, but in the default option with AC_ARG (BTW, it should be an ENABLE, not a WITH), something like that (not tested):
AC_ARG_ENABLE(
[mptcp],
[AS_HELP_STRING([--disable-mptcp], [disable ultipath TCP support @<:@default=yes@:>@ on Linux only])],
,
[
case "$host" in
*-*-linux*) enable_mptcp="yes";;
*) enable_mptcp="no" ;;
esac
]
)
src/openvpn/options.c
Outdated
| @@ -135,6 +135,9 @@ static const char usage_message[] = | |||
| " udp6, tcp6-server, tcp6-client\n" | |||
| "--proto-force p : only consider protocol p in list of connection profiles.\n" | |||
| " p = udp or tcp\n" | |||
| #if defined(TARGET_LINUX) && defined(ENABLE_MPTCP) | |||
There was a problem hiding this comment.
(see my comment above: you don't need
TARGET_LINUX, same below at a few places)
src/openvpn/options.c
Outdated
| @@ -135,6 +135,9 @@ static const char usage_message[] = | |||
| " udp6, tcp6-server, tcp6-client\n" | |||
| "--proto-force p : only consider protocol p in list of connection profiles.\n" | |||
| " p = udp or tcp\n" | |||
| #if defined(TARGET_LINUX) && defined(ENABLE_MPTCP) | |||
| "--multipath : Enable Multipath TCP on the TCP connections.\n" | |||
There was a problem hiding this comment.
why placing it here? Just to be next to the
--protooption? (it would make sense but just to know, might be interesting to add this in the commit message)
(same as below: I would recommend to use
--mptcpto avoid confusions)(or maybe
--proto-mptcp?)
or modify the behaviour of
--proto? but then you will need to allowmptcp{,4,6}{,-client,server}, maybe not what we want?
src/openvpn/socket.c
Outdated
| if(sock->sd == -1) | ||
| { | ||
|
|
||
| msg(M_NONFATAL, "Can't resolve MPTCP socket, fallback to TCP !"); |
There was a problem hiding this comment.
(just saw this after my previous comment. Good idea to fallback)
(PS: in English, you don't add a space before
!)
Signed-off-by: Arınç ÜNAL <[email protected]>
|
@matttbe could you take a look at the second patch? If it looks fine by you, I'll squash to a single commit for further review from you. |
| @@ -38,11 +38,9 @@ | |||
| #include "memdbg.h" | |||
|
|
|||
|
|
|||
| #if defined(TARGET_LINUX) && defined(ENABLE_MPTCP) | |||
| #ifndef IPPROTO_MPTCP | |||
| #define IPPROTO_MPTCP 262 | |||
| #endif | |||
There was a problem hiding this comment.
I guess you can remove that as it is now defined in socket.h
| #else | ||
| if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) | ||
| #endif | ||
| /* TODO: provide access to the options structure */ |
There was a problem hiding this comment.
Sorry, I don't think I can help with that. Maybe this option has to be duplicated in a different structure here (similar to sock->info.mptcp = o->enable_mptcp; for socket.c), or passed by the caller.
But before doing that, do you know when this function is used? Is it used when connecting to OpenVPN server via an HTTP proxy? Maybe MPTCP support for this can be added later to that, no?
| @@ -308,6 +308,13 @@ AC_ARG_WITH( | |||
| [with_openssl_engine="auto"] | |||
| ) | |||
|
|
|||
| AC_ARG_WITH( | |||
There was a problem hiding this comment.
see my other comment: AC_ARG_ENABLE I suppose
| [mptcp], | ||
| [AS_HELP_STRING([--disable-mptcp], [Disable Multipath TCP support @<:@default=yes@:>@])], | ||
| , | ||
| [enable_mptcp="yes"] |
There was a problem hiding this comment.
see my other comment, here it should only be enabled by default on Linux
| dnl | ||
| case "$host" in | ||
| *-*-linux*) | ||
| AC_DEFINE(ENABLE_MPTCP, 1, [Enable checking Multipath TCP support]) |
There was a problem hiding this comment.
see my other comment, it should not be done here, but above, in the AC_ARG
| { | ||
|
|
||
| msg(M_NONFATAL, "Can't resolve MPTCP socket, fallback to TCP!"); | ||
| sock->sd = create_socket_tcp(addr); |
There was a problem hiding this comment.
I guess the socket is created when starting OpenVPN, right? (I mean: I guess the client will not request the server to create an MPTCP socket, right?)
In this case, it means that the person who is launching OpenVPN has explicitly requested to use MPTCP: then better not to fallback in case of error, but fail, no? If yes, then it might be better to adapt create_socket_tcp() to support MPTCP, e.g. by adding a new parameter, or by setting addr->ai_protocol = IPPROTO_MPTCP is MPTCP has been requested (but that might have other consequences if addr is used later on, to be confirmed)
In my ideal world, MPTCP would be enabled by default, and used only if the client requested it. But I don't know if OpenVPN devs will accept that.
(while at it, but not directly related to ↑, you might need to do something else for DCO, because they don't support MPTCP yet: OpenVPN/ovpn-dco#60 )
No description provided.