Skip to contentSkip to conversation

Do not set the buffer size by default but rely on the operation system default.

PATCH Superseded Arne Schwabe 5 participants 7 messages Oct 11, 2015 – Oct 19, 2015

lore
Share & tools
lore

Arne Schwabe

author
#0 lore raw

PATCH 1/1 Do not set the buffer size by default but rely on the operation system default.

---
 ChangeLog             | 3 +++
 doc/openvpn.8         | 4 ++--
 src/openvpn/options.c | 4 ----
 3 files changed, 5 insertions(+), 6 deletions(-)
ChangeLog +3 -0
index 7b945c8..9642adf 100644
@@ -1,6 +1,9 @@
 OpenVPN Change Log
 Copyright (C) 2002-2012 OpenVPN Technologies, Inc. <[email protected]...>
 
+2015.xx.xx -- Version 2.4.0
+	- sndbuf and recvbuf default now to OS default instead of 65k
+
 2012.09.12 -- Version 2.3_beta1
 Arne Schwabe (7):
       Fixes error: --key fails with EXTERNAL_PRIVATE_KEY: No such file or directory if --management-external-key is used
doc/openvpn.8 +2 -2
index 3a86409..b6d5aed 100644
@@ -1433,12 +1433,12 @@ connection problems) with the following options:
 .TP
 .B \-\-sndbuf size
 Set the TCP/UDP socket send buffer size.
-Currently defaults to 65536 bytes.
+Defaults to operation system default.
 .\"*********************************************************
 .TP
 .B \-\-rcvbuf size
 Set the TCP/UDP socket receive buffer size.
-Currently defaults to 65536 bytes.
+Defaults to operation system default.
 .\"*********************************************************
 .TP
 .B \-\-mark value
src/openvpn/options.c +0 -4
index 6823167..2b0af34 100644
@@ -785,10 +785,6 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_FEATURE_TUN_PERSIST
   o->persist_mode = 1;
 #endif
-#ifndef WIN32
-  o->rcvbuf = 65536;
-  o->sndbuf = 65536;
-#endif
 #ifdef TARGET_LINUX
   o->tuntap_options.txqueuelen = 100;
 #endif
Signature · 1 line
1.9.5 (Apple Git-50.3)
#1 lore raw
Quoted 11 lines · Arne Schwabe
---
 ChangeLog             | 3 +++
 doc/openvpn.8         | 4 ++--
 src/openvpn/options.c | 4 ----
 3 files changed, 5 insertions(+), 6 deletions(-)
ChangeLog +0 -0
index 7b945c8..9642adf 100644

ChangeLog line 6

Quoted 6 lines
ChangeLog +3 -0
@@ -1,6 +1,9 @@
 OpenVPN Change Log
 Copyright (C) 2002-2012 OpenVPN Technologies, Inc. <[email protected]...>
 
+2015.xx.xx -- Version 2.4.0
+       - sndbuf and recvbuf default now to OS default instead of 65k
+

Nice, changelog entries! But, 65k should be 64k.

Quoted 5 lines
src/openvpn/options.c +0 -0
index 6823167..2b0af34 100644

src/openvpn/options.c init_options (struct options *o, const bool init_gc) · line 790

Quoted 10 lines
src/openvpn/options.c +0 -4
@@ -785,10 +785,6 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_FEATURE_TUN_PERSIST
   o->persist_mode = 1;
 #endif
-#ifndef WIN32
-  o->rcvbuf = 65536;
-  o->sndbuf = 65536;
-#endif
 #ifdef TARGET_LINUX
   o->tuntap_options.txqueuelen = 100;
 #endif

Am I right that it was tested that this indeed results in using operating system defaults?

-Steffan

#2 lore raw
Quoted 5 lines · Steffan Karger

On Sun, Oct 11, 2015 at 3:47 PM, Arne Schwabe <[email protected]...> wrote:
Nice, changelog entries! But, 65k should be 64k.

Am I right that it was tested that this indeed results in using operating system defaults?

Yes.

Quoted 8 lines

-Steffan

------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

#3 lore raw
In reply to Steffan Karger

Hi,

Quoted 11 lines · Steffan Karger

ChangeLog line 6

Quoted 9 lines
ChangeLog +3 -0
@@ -1,6 +1,9 @@
 OpenVPN Change Log
 Copyright (C) 2002-2012 OpenVPN Technologies, Inc. <[email protected]...>
 
+2015.xx.xx -- Version 2.4.0
+       - sndbuf and recvbuf default now to OS default instead of 65k
+

Nice, changelog entries! But, 65k should be 64k.

ChangeLog entries are good, but I disagree on the format (sorry) - what we have in ChangeLog *today* is basically "git shortlog $tag..$HEAD" when doing a release - so I think we should not mix the "user visible changes" in here, but maintain a separate file, eg. "CHANGES(.rst, .md)", for these.

Alternative, rename ChangeLog to "GitShortlog.txt" and put the user visible changes into ChangeLog - but please do not mix.

(The rest of the patch is fine, but should mention trac #461 in the commit message - I'll amend if OK)

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#4 lore raw

On 11/10/15 19:44, Gert Doering wrote:
[...snip...

Quoted 5 lines

ChangeLog entries are good, but I disagree on the format (sorry) - what we have in ChangeLog *today* is basically "git shortlog $tag..$HEAD" when doing a release - so I think we should not mix the "user visible changes" in here, but maintain a separate file, eg. "CHANGES(.rst, .md)", for these.

Agreed! I see that the files Samuli added regarding contribution have .rst. I don't care if it is .rst or .md. But lets keep the same extension/format for CHANGES too.

Quoted 2 lines

Alternative, rename ChangeLog to "GitShortlog.txt" and put the user visible changes into ChangeLog - but please do not mix.

Agreeing to the mixing. I think we should keep ChangeLog as is, as I believe that resembles the format used since the very beginning. Even though the verbosity have increased since we started using git and git shortlog. Some packagers or other interested may already have routines in place for grokking through our current ChangeLog file.

Signature · 3 lines
kind regards,

David Sommerseth
author
#5 lore raw

PATCH v2 1/1 Do not set the buffer size by default but rely on the operation system default.

In reply to Arne Schwabe

Also remove SOCKET_SND_RCV_BUF_MAX since limiting the buffer to 1000k is arbitrary and all OSes impose a maximum that can be set anyway.

closes trac ticket #461

V2: SOCKET_SND_RCV_BUF_MAX removal

---
 doc/openvpn.8         |  4 ++--
 src/openvpn/options.c |  4 ----
 src/openvpn/socket.c  | 16 +++++-----------
 src/openvpn/socket.h  |  5 -----
 4 files changed, 7 insertions(+), 22 deletions(-)
doc/openvpn.8 +2 -2
index b8fb2e5..f305c58 100644
@@ -1418,12 +1418,12 @@ connection problems) with the following options:
 .TP
 .B \-\-sndbuf size
 Set the TCP/UDP socket send buffer size.
-Currently defaults to 65536 bytes.
+Defaults to operation system default.
 .\"*********************************************************
 .TP
 .B \-\-rcvbuf size
 Set the TCP/UDP socket receive buffer size.
-Currently defaults to 65536 bytes.
+Defaults to operation system default.
 .\"*********************************************************
 .TP
 .B \-\-mark value
src/openvpn/options.c +0 -4
index 55db81a..086dcea 100644
@@ -783,10 +783,6 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_FEATURE_TUN_PERSIST
   o->persist_mode = 1;
 #endif
-#ifndef WIN32
-  o->rcvbuf = 65536;
-  o->sndbuf = 65536;
-#endif
 #ifdef TARGET_LINUX
   o->tuntap_options.txqueuelen = 100;
 #endif
src/openvpn/socket.c +5 -11
index b4032b8..dd8ea29 100644
@@ -624,12 +624,9 @@ static void
 socket_set_sndbuf (int sd, int size)
 {
 #if defined(HAVE_SETSOCKOPT) && defined(SOL_SOCKET) && defined(SO_SNDBUF)
-  if (size > 0 && size < SOCKET_SND_RCV_BUF_MAX)
+  if (setsockopt (sd, SOL_SOCKET, SO_SNDBUF, (void *) &size, sizeof (size)) != 0)
     {
-      if (setsockopt (sd, SOL_SOCKET, SO_SNDBUF, (void *) &size, sizeof (size)) != 0)
-	{
-	  msg (M_WARN, "NOTE: setsockopt SO_SNDBUF=%d failed", size);
-	}
+      msg (M_WARN, "NOTE: setsockopt SO_SNDBUF=%d failed", size);
     }
 #endif
 }

@@ -653,13 +650,10 @@ static bool
 socket_set_rcvbuf (int sd, int size)
 {
 #if defined(HAVE_SETSOCKOPT) && defined(SOL_SOCKET) && defined(SO_RCVBUF)
-  if (size > 0 && size < SOCKET_SND_RCV_BUF_MAX)
+  if (setsockopt (sd, SOL_SOCKET, SO_RCVBUF, (void *) &size, sizeof (size)) != 0)
     {
-      if (setsockopt (sd, SOL_SOCKET, SO_RCVBUF, (void *) &size, sizeof (size)) != 0)
-	{
-	  msg (M_WARN, "NOTE: setsockopt SO_RCVBUF=%d failed", size);
-	  return false;
-	}
+      msg (M_WARN, "NOTE: setsockopt SO_RCVBUF=%d failed", size);
+      return false;
     }
   return true;
 #endif
src/openvpn/socket.h +0 -5
index 30b6ace..da224dd 100644
@@ -42,11 +42,6 @@
 #define OPENVPN_PORT "1194"
 
 /*
- * Maximum size passed passed to setsockopt SNDBUF/RCVBUF
- */
-#define SOCKET_SND_RCV_BUF_MAX 1000000
-
-/*
  * Number of seconds that "resolv-retry infinite"
  * represents.
  */
Signature · 1 line
2.3.8 (Apple Git-58)
#6 lore raw

ACK.

Your patch has been applied to the master and release/2.3 branch.

commit f0b64e5dc00f35e3b0fe8c53a316ee74c9cbf15f (master) commit c72dbb8b470ab7b25fc74e41aed4212db48a9d2f (release/2.3)

Author: Arne Schwabe
Date:   Thu Oct 15 16:38:38 2015 +0200
     Do not set the buffer size by default but rely on the operation system default.

     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10280
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering