Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1454?usp=email
to review the following change.
Change subject: Remove --mtu-test
......................................................................
Remove --mtu-test
This option doesn't seem to have worked right at least since
the merge of commit fb76954a232decc86e071a76861c1e4fdd2507ab
which was merged before 2.6.0. Clearly noone cares enough
about this feature maintain or even test it. So let's not
pretend and get rid of it.
Change-Id: Ic1e84ef6fb613851861a695bfbdbc1c32970a398
Signed-off-by: Frank Lichtenheld <[email protected]>
---
M doc/man-sections/link-options.rst
M doc/man-sections/unsupported-options.rst
M src/openvpn/forward.c
M src/openvpn/init.c
M src/openvpn/occ.c
M src/openvpn/occ.h
M src/openvpn/openvpn.h
M src/openvpn/options.c
8 files changed, 5 insertions(+), 338 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/54/1454/1
diff --git a/doc/man-sections/link-options.rst
b/doc/man-sections/link-options.rst
index edda1ca..062be45 100644
--- a/doc/man-sections/link-options.rst
+++ b/doc/man-sections/link-options.rst
@@ -222,13 +222,6 @@
:code:`yes` Always DF (Don't Fragment)
---mtu-test
- To empirically measure MTU on connection startup, add the ``--mtu-test``
- option to your configuration. OpenVPN will send ping packets of various
- sizes to the remote peer and measure the largest packets which were
- successfully received. The ``--mtu-test`` process normally takes about 3
- minutes to complete.
-
--nobind
Do not bind to local address and port. The IP stack will allocate a
dynamic port for returning packets. Since the value of the dynamic port
diff --git a/doc/man-sections/unsupported-options.rst
b/doc/man-sections/unsupported-options.rst
index f1332f3..d1bcf6e 100644
--- a/doc/man-sections/unsupported-options.rst
+++ b/doc/man-sections/unsupported-options.rst
@@ -35,6 +35,9 @@
--max-routes
Removed in OpenVPN 2.4. The limit was removed.
+--mtu-test
+ This option was broken in OpenVPN 2.6 and was removed in OpenVPN 2.7.
+
--ncp-disable
Removed in OpenVPN 2.6. This option mainly served a role as debug option
when NCP was first introduced. It should no longer be necessary.
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 492e667..586809a 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -810,9 +810,6 @@
/* Should we send an OCC_REQUEST message? */
check_send_occ_req(c);
- /* Should we send an MTU load test? */
- check_send_occ_load_test(c);
-
/* Should we send an OCC_EXIT message to remote? */
if (c->c2.explicit_exit_notification_time_wait)
{
@@ -993,11 +990,6 @@
{
c->c2.link_read_bytes += c->c2.buf.len;
link_read_bytes_global += c->c2.buf.len;
- c->c2.original_recv_size = c->c2.buf.len;
- }
- else
- {
- c->c2.original_recv_size = 0;
}
#ifdef ENABLE_DEBUG
@@ -1168,8 +1160,6 @@
if (c->c2.buf.len > 0)
{
c->c2.link_read_bytes_auth += c->c2.buf.len;
- c->c2.max_recv_size_local =
- max_int(c->c2.original_recv_size, c->c2.max_recv_size_local);
}
/* Did we just receive an openvpn ping packet? */
@@ -1825,7 +1815,6 @@
if (size > 0)
{
- c->c2.max_send_size_local = max_int(size,
c->c2.max_send_size_local);
c->c2.link_write_bytes += size;
link_write_bytes_global += size;
}
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index ee198ce..ee5b427 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1378,12 +1378,6 @@
event_timeout_init(&c->c2.occ_interval, OCC_INTERVAL_SECONDS, now);
}
- if (c->options.mtu_test)
- {
- event_timeout_init(&c->c2.occ_mtu_load_test_interval,
OCC_MTU_LOAD_INTERVAL_SECONDS,
- now);
- }
-
/* initialize packet_id persistence timer */
if (c->options.packet_id_file)
{
diff --git a/src/openvpn/occ.c b/src/openvpn/occ.c
index 78013ae..6730c39 100644
--- a/src/openvpn/occ.c
+++ b/src/openvpn/occ.c
@@ -54,92 +54,6 @@
const uint8_t occ_magic[] = { 0x28, 0x7f, 0x34, 0x6b, 0xd4, 0xef, 0x7a, 0x81,
0x2d, 0x56, 0xb8, 0xd3, 0xaf, 0xc5, 0x45, 0x9c };
-static const struct mtu_load_test mtu_load_test_sequence[] = {
-
- { OCC_MTU_LOAD_REQUEST, -1000 },
- { OCC_MTU_LOAD, -1000 },
- { OCC_MTU_LOAD_REQUEST, -1000 },
- { OCC_MTU_LOAD, -1000 },
- { OCC_MTU_LOAD_REQUEST, -1000 },
- { OCC_MTU_LOAD, -1000 },
-
- { OCC_MTU_LOAD_REQUEST, -750 },
- { OCC_MTU_LOAD, -750 },
- { OCC_MTU_LOAD_REQUEST, -750 },
- { OCC_MTU_LOAD, -750 },
- { OCC_MTU_LOAD_REQUEST, -750 },
- { OCC_MTU_LOAD, -750 },
-
- { OCC_MTU_LOAD_REQUEST, -500 },
- { OCC_MTU_LOAD, -500 },
- { OCC_MTU_LOAD_REQUEST, -500 },
- { OCC_MTU_LOAD, -500 },
- { OCC_MTU_LOAD_REQUEST, -500 },
- { OCC_MTU_LOAD, -500 },
-
- { OCC_MTU_LOAD_REQUEST, -400 },
- { OCC_MTU_LOAD, -400 },
- { OCC_MTU_LOAD_REQUEST, -400 },
- { OCC_MTU_LOAD, -400 },
- { OCC_MTU_LOAD_REQUEST, -400 },
- { OCC_MTU_LOAD, -400 },
-
- { OCC_MTU_LOAD_REQUEST, -300 },
- { OCC_MTU_LOAD, -300 },
- { OCC_MTU_LOAD_REQUEST, -300 },
- { OCC_MTU_LOAD, -300 },
- { OCC_MTU_LOAD_REQUEST, -300 },
- { OCC_MTU_LOAD, -300 },
-
- { OCC_MTU_LOAD_REQUEST, -200 },
- { OCC_MTU_LOAD, -200 },
- { OCC_MTU_LOAD_REQUEST, -200 },
- { OCC_MTU_LOAD, -200 },
- { OCC_MTU_LOAD_REQUEST, -200 },
- { OCC_MTU_LOAD, -200 },
-
- { OCC_MTU_LOAD_REQUEST, -150 },
- { OCC_MTU_LOAD, -150 },
- { OCC_MTU_LOAD_REQUEST, -150 },
- { OCC_MTU_LOAD, -150 },
- { OCC_MTU_LOAD_REQUEST, -150 },
- { OCC_MTU_LOAD, -150 },
-
- { OCC_MTU_LOAD_REQUEST, -100 },
- { OCC_MTU_LOAD, -100 },
- { OCC_MTU_LOAD_REQUEST, -100 },
- { OCC_MTU_LOAD, -100 },
- { OCC_MTU_LOAD_REQUEST, -100 },
- { OCC_MTU_LOAD, -100 },
-
- { OCC_MTU_LOAD_REQUEST, -50 },
- { OCC_MTU_LOAD, -50 },
- { OCC_MTU_LOAD_REQUEST, -50 },
- { OCC_MTU_LOAD, -50 },
- { OCC_MTU_LOAD_REQUEST, -50 },
- { OCC_MTU_LOAD, -50 },
-
- { OCC_MTU_LOAD_REQUEST, 0 },
- { OCC_MTU_LOAD, 0 },
- { OCC_MTU_LOAD_REQUEST, 0 },
- { OCC_MTU_LOAD, 0 },
- { OCC_MTU_LOAD_REQUEST, 0 },
- { OCC_MTU_LOAD, 0 },
-
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
- { OCC_MTU_REQUEST, 0 },
-
- { -1, 0 }
-};
-
void
check_send_occ_req_dowork(struct context *c)
{
@@ -153,8 +67,7 @@
*/
msg(D_SHOW_OCC,
"NOTE: failed to obtain options consistency info from peer -- "
- "this could occur if the remote peer is running a version of "
PACKAGE_NAME
- " before 1.5-beta8 or if there is a network connectivity
problem, and will not necessarily prevent " PACKAGE_NAME
+ "this could occur if there is a network connectivity problem,
and will not necessarily prevent " PACKAGE_NAME
" from running (" counter_format " bytes received from peer, "
counter_format
" bytes authenticated data channel traffic) -- you can disable
the options consistency "
"check with --disable-occ.",
@@ -174,45 +87,6 @@
}
}
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wconversion"
-#endif
-
-void
-check_send_occ_load_test_dowork(struct context *c)
-{
- if (connection_established(c))
- {
- const struct mtu_load_test *entry;
-
- if (!c->c2.occ_mtu_load_n_tries)
- {
- msg(M_INFO,
- "NOTE: Beginning empirical MTU test -- results should be
available in 3 to 4 minutes.");
- }
-
- entry = &mtu_load_test_sequence[c->c2.occ_mtu_load_n_tries++];
- if (entry->op >= 0)
- {
- c->c2.occ_op = entry->op;
- size_t payload_size =
- frame_calculate_payload_size(&c->c2.frame, &c->options,
&c->c1.ks.key_type);
- size_t header_size =
- frame_calculate_protocol_header_size(&c->c1.ks.key_type,
&c->options, false);
-
- c->c2.occ_mtu_load_size = payload_size + header_size;
- }
- else
- {
- msg(M_INFO, "NOTE: failed to empirically measure MTU (requires "
PACKAGE_NAME
- " 1.5 or higher at other end of connection).");
- event_timeout_clear(&c->c2.occ_mtu_load_test_interval);
- c->c2.occ_mtu_load_n_tries = 0;
- }
- }
-}
-
void
check_send_occ_msg_dowork(struct context *c)
{
@@ -252,84 +126,6 @@
doit = true;
break;
- case OCC_MTU_REQUEST:
- if (!buf_write_u8(&c->c2.buf, OCC_MTU_REQUEST))
- {
- break;
- }
- dmsg(D_PACKET_CONTENT, "SENT OCC_MTU_REQUEST");
- doit = true;
- break;
-
- case OCC_MTU_REPLY:
- if (!buf_write_u8(&c->c2.buf, OCC_MTU_REPLY))
- {
- break;
- }
- if (!buf_write_u16(&c->c2.buf, c->c2.max_recv_size_local))
- {
- break;
- }
- if (!buf_write_u16(&c->c2.buf, c->c2.max_send_size_local))
- {
- break;
- }
- dmsg(D_PACKET_CONTENT, "SENT OCC_MTU_REPLY");
- doit = true;
- break;
-
- case OCC_MTU_LOAD_REQUEST:
- if (!buf_write_u8(&c->c2.buf, OCC_MTU_LOAD_REQUEST))
- {
- break;
- }
- if (!buf_write_u16(&c->c2.buf, c->c2.occ_mtu_load_size))
- {
- break;
- }
- dmsg(D_PACKET_CONTENT, "SENT OCC_MTU_LOAD_REQUEST");
- doit = true;
- break;
-
- case OCC_MTU_LOAD:
- {
- int need_to_add;
-
- if (!buf_write_u8(&c->c2.buf, OCC_MTU_LOAD))
- {
- break;
- }
- size_t proto_hdr, payload_hdr;
- const struct key_type *kt = &c->c1.ks.key_type;
-
- /* OCC message have comp/fragment headers but not ethernet headers
*/
- payload_hdr = frame_calculate_payload_overhead(0, &c->options, kt);
-
- /* Since we do not know the payload size we just pass 0 as size
here */
- proto_hdr = frame_calculate_protocol_header_size(kt, &c->options,
false);
-
- need_to_add = min_int(c->c2.occ_mtu_load_size,
c->c2.frame.buf.payload_size)
- - OCC_STRING_SIZE - sizeof(uint8_t) /* occ opcode */
- - payload_hdr - proto_hdr;
-
- while (need_to_add > 0)
- {
- /*
- * Fill the load test packet with pseudo-random bytes.
- */
- if (!buf_write_u8(&c->c2.buf, get_random() & 0xFF))
- {
- break;
- }
- --need_to_add;
- }
- dmsg(D_PACKET_CONTENT, "SENT OCC_MTU_LOAD
min_int(%d,%d)-%d-%d-%d-%d) size=%d",
- c->c2.occ_mtu_load_size, c->c2.frame.buf.payload_size,
OCC_STRING_SIZE,
- (int)sizeof(uint8_t), (int)payload_hdr, (int)proto_hdr,
BLEN(&c->c2.buf));
- doit = true;
- }
- break;
-
case OCC_EXIT:
if (!buf_write_u8(&c->c2.buf, OCC_EXIT))
{
@@ -352,10 +148,6 @@
c->c2.occ_op = -1;
}
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
void
process_received_occ_msg(struct context *c)
{
@@ -367,20 +159,6 @@
c->c2.occ_op = OCC_REPLY;
break;
- case OCC_MTU_REQUEST:
- dmsg(D_PACKET_CONTENT, "RECEIVED OCC_MTU_REQUEST");
- c->c2.occ_op = OCC_MTU_REPLY;
- break;
-
- case OCC_MTU_LOAD_REQUEST:
- dmsg(D_PACKET_CONTENT, "RECEIVED OCC_MTU_LOAD_REQUEST");
- c->c2.occ_mtu_load_size = buf_read_u16(&c->c2.buf);
- if (c->c2.occ_mtu_load_size >= 0)
- {
- c->c2.occ_op = OCC_MTU_LOAD;
- }
- break;
-
case OCC_REPLY:
dmsg(D_PACKET_CONTENT, "RECEIVED OCC_REPLY");
if (c->options.occ && !TLS_MODE(c) && c->c2.options_string_remote)
@@ -395,30 +173,6 @@
event_timeout_clear(&c->c2.occ_interval);
break;
- case OCC_MTU_REPLY:
- dmsg(D_PACKET_CONTENT, "RECEIVED OCC_MTU_REPLY");
- c->c2.max_recv_size_remote = buf_read_u16(&c->c2.buf);
- c->c2.max_send_size_remote = buf_read_u16(&c->c2.buf);
- if (c->options.mtu_test && c->c2.max_recv_size_remote > 0
- && c->c2.max_send_size_remote > 0)
- {
- msg(M_INFO,
- "NOTE: Empirical MTU test completed [Tried,Actual]
local->remote=[%d,%d] remote->local=[%d,%d]",
- c->c2.max_send_size_local, c->c2.max_recv_size_remote,
- c->c2.max_send_size_remote, c->c2.max_recv_size_local);
- if (!c->options.ce.fragment &&
(proto_is_dgram(c->options.ce.proto))
- && c->c2.max_send_size_local > TUN_MTU_MIN
- && (c->c2.max_recv_size_remote < c->c2.max_send_size_local
- || c->c2.max_recv_size_local <
c->c2.max_send_size_remote))
- {
- msg(M_INFO,
- "NOTE: This connection is unable to accommodate a UDP
packet size of %d. Consider using --fragment or --mssfix options as a
workaround.",
- c->c2.max_send_size_local);
- }
- }
- event_timeout_clear(&c->c2.occ_mtu_load_test_interval);
- break;
-
case OCC_EXIT:
dmsg(D_STREAM_ERRORS, "OCC exit message received by peer");
register_signal(c->sig, SIGUSR1, "remote-exit");
diff --git a/src/openvpn/occ.h b/src/openvpn/occ.h
index 369d94e..2b816ec 100644
--- a/src/openvpn/occ.h
+++ b/src/openvpn/occ.h
@@ -45,39 +45,12 @@
#define OCC_INTERVAL_SECONDS 10
#define OCC_N_TRIES 12
-/*
- * Other OCC protocol opcodes used to estimate the MTU empirically.
- */
-#define OCC_MTU_LOAD_REQUEST 2 /* Ask peer to send a big packet to us */
-#define OCC_MTU_LOAD 3 /* Send a big packet to peer */
-#define OCC_MTU_REQUEST \
- 4 /* Ask peer to tell us the largest \
- * packet it has received from us so far */
-#define OCC_MTU_REPLY 5 /* Send largest packet size to peer */
-
-/*
- * Process one command from mtu_load_test_sequence
- * once every n seconds, if --mtu-test is specified.
- */
-#define OCC_MTU_LOAD_INTERVAL_SECONDS 3
-
+/* 2 - 5 removed (OCC MTU Test) */
/*
* Send an exit message to remote.
*/
#define OCC_EXIT 6
-/*
- * Used to conduct a load test command sequence
- * of UDP connection for empirical MTU measurement.
- */
-struct mtu_load_test
-{
- int op; /* OCC opcode to send to peer */
- int delta; /* determine packet size to send by using
- * this delta against currently
- * configured MTU */
-};
-
extern const uint8_t occ_magic[];
static inline bool
@@ -90,8 +63,6 @@
void check_send_occ_req_dowork(struct context *c);
-void check_send_occ_load_test_dowork(struct context *c);
-
void check_send_occ_msg_dowork(struct context *c);
/*
@@ -119,20 +90,6 @@
}
/*
- * Should we send an MTU load test?
- */
-static inline void
-check_send_occ_load_test(struct context *c)
-{
- if (event_timeout_defined(&c->c2.occ_mtu_load_test_interval)
- && event_timeout_trigger(&c->c2.occ_mtu_load_test_interval,
&c->c2.timeval,
- (!TO_LINK_DEF(c) && c->c2.occ_op < 0) ?
ETT_DEFAULT : 0))
- {
- check_send_occ_load_test_dowork(c);
- }
-}
-
-/*
* Should we send an OCC message?
*/
static inline void
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index 9325e21..c1cdb5a 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -301,23 +301,6 @@
struct event_timeout occ_interval;
/*
- * Keep track of maximum packet size received so far
- * (of authenticated packets).
- */
- int original_recv_size; /* temporary */
- int max_recv_size_local; /* max packet size received */
- int max_recv_size_remote; /* max packet size received by remote */
- int max_send_size_local; /* max packet size sent */
- int max_send_size_remote; /* max packet size sent by remote */
-
-
- /* remote wants us to send back a load test packet of this size */
- int occ_mtu_load_size;
-
- struct event_timeout occ_mtu_load_test_interval;
- int occ_mtu_load_n_tries;
-
- /*
* TLS-mode crypto objects.
*/
struct tls_multi *tls_multi; /**< TLS state structure for this VPN
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 34af0d3..e3bcbc7 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -305,7 +305,6 @@
" 'no' -- Never send DF (Don't Fragment) frames\n"
" 'maybe' -- Use per-route hints\n"
" 'yes' -- Always DF (Don't Fragment)\n"
- "--mtu-test : Empirically measure and report MTU.\n"
#ifdef ENABLE_FRAGMENT
"--fragment max : Enable internal datagram fragmentation so that no UDP\n"
" datagrams are sent which are larger than max bytes.\n"
@@ -6479,11 +6478,6 @@
VERIFY_PERMISSION(OPT_P_MTU | OPT_P_CONNECTION);
options->ce.mtu_discover_type = translate_mtu_discover_type_name(p[1]);
}
- else if (streq(p[0], "mtu-test") && !p[1])
- {
- VERIFY_PERMISSION(OPT_P_GENERAL);
- options->mtu_test = true;
- }
else if (streq(p[0], "nice") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_NICE);
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1454?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ic1e84ef6fb613851861a695bfbdbc1c32970a398
Gerrit-Change-Number: 1454
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
