Change the hold command to communicate the time that OpenVPN would wait to the UI.

PATCH Superseded Arne Schwabe 4 participants 6 messages Jul 11, 2016 – Oct 13, 2016

Share & tools

Arne Schwabe

author

2016-07-11 14:50 UTC #0 lore raw

PATCH 1/1 Change the hold command to communicate the time that OpenVPN would wait to the UI.

Before the connect-retry change to do exponential backup this was not necessary since the time was fixed. With the exponential backoff the UI needs either to implement its own exponential backoff mechansim or needs a way of knowing the value of OpenVPN internal mechansim.

---
 doc/management-notes.txt |  7 +++++--
 src/openvpn/init.c       | 17 ++++++++++-------
 src/openvpn/manage.c     |  8 ++++++--
 src/openvpn/manage.h     |  2 +-
 4 files changed, 22 insertions(+), 12 deletions(-)

doc/management-notes.txt +5 -2

index f68f3db..eb857ab 100644
@@ -168,9 +168,12 @@ be reset by restarts.
 
 OpenVPN will indicate that it is in a hold state by
 sending a real-time notification to the management
-client:
+client, the parameter indicates how long OpenVPN would
+wait without UI (as influenced connect-retry exponential
+backoff. The UI need to wait for releasing the hold if it
+wants similar behavior:
 
-  >HOLD:Waiting for hold release
+  >HOLD:Waiting for hold release:10
 
 Command examples:

src/openvpn/init.c +10 -7

index 3af2bdc..ef64b39 100644
@@ -1960,16 +1960,17 @@ do_deferred_options (struct context *c, const unsigned int found)
 }
 
 /*
- * Possible hold on initialization
+ * Possible hold on initialization, holdtime is the 
+ * time OpenVPN would wait without management
  */
 static bool
-do_hold (void)
+do_hold (int holdtime)
 {
 #ifdef ENABLE_MANAGEMENT
   if (management)
     {
       /* block until management hold is released */
-      if (management_hold (management))
+        if (management_hold (management, holdtime))
 	return true;
     }
 #endif

@@ -2012,9 +2013,11 @@ socket_restart_pause (struct context *c)
   c->persist.restart_sleep_seconds = 0;
 
   /* do managment hold on context restart, i.e. second, third, fourth, etc. initialization */
-  if (do_hold ())
+  if (do_hold (sec))
+  {
     sec = 0;
-
+  }
+  
   if (sec)
     {
       msg (D_RESTART, "Restart pause, %d second(s)", sec);

@@ -2031,7 +2034,7 @@ do_startup_pause (struct context *c)
   if (!c->first_time)
     socket_restart_pause (c);
   else
-    do_hold (); /* do management hold on first context initialization */
+    do_hold (0); /* do management hold on first context initialization */
 }
 
 /*

@@ -3435,7 +3438,7 @@ open_management (struct context *c)
 	    }
 
 	  /* initial management hold, called early, before first context initialization */
-	  do_hold ();
+	  do_hold (0);
 	  if (IS_SIG (c))
 	    {
 	      msg (M_WARN, "Signal received from management interface, exiting");

src/openvpn/manage.c +6 -2

index dcb1bc1..206aaf4 100644
@@ -3332,12 +3332,13 @@ management_should_daemonize (struct management *man)
  * Return true if the caller should not sleep for an additional time interval.
  */
 bool
-management_hold (struct management *man)
+management_hold (struct management *man, int holdtime)
 {
   if (management_would_hold (man))
     {
       volatile int signal_received = 0;
       const bool standalone_disabled_save = man->persist.standalone_disabled;
+      struct gc_arena gc = gc_new ();
 
       man->persist.standalone_disabled = false; /* This is so M_CLIENT messages will be correctly passed through msg() */
       man->persist.special_state_msg = NULL;

@@ -3347,7 +3348,9 @@ management_hold (struct management *man)
 
       if (!signal_received)
 	{
-	  man->persist.special_state_msg = ">HOLD:Waiting for hold release";
+      struct buffer out = alloc_buf_gc (128, &gc);
+      buf_printf (&out, ">HOLD:Waiting for hold release:%d", holdtime);
+	  man->persist.special_state_msg = BSTR (&out);
 	  msg (M_CLIENT, "%s", man->persist.special_state_msg);
 
 	  /* run command processing event loop until we get our username/password */

@@ -3366,6 +3369,7 @@ management_hold (struct management *man)
       man->persist.special_state_msg = NULL;
       man->settings.mansig &= ~MANSIG_IGNORE_USR1_HUP;
 
+      gc_free (&gc);
       return true;
     }
   return false;

src/openvpn/manage.h +1 -1

index 988600f..50db38c 100644
@@ -396,7 +396,7 @@ int managment_android_persisttun_action (struct management *man);
 
 bool management_should_daemonize (struct management *man);
 bool management_would_hold (struct management *man);
-bool management_hold (struct management *man);
+bool management_hold (struct management *man, int holdtime);
 
 void management_event_loop_n_seconds (struct management *man, int sec);

Signature · 1 line

2.7.4 (Apple Git-66)

Selva Nair

2016-07-11 20:14 UTC #1 lore raw

Hi,

Quoted 8 lines · Arne Schwabe

Before the connect-retry change to do exponential backup this was not necessary
since the time was fixed. With the exponential backoff the UI needs either to
implement its own exponential backoff mechansim or needs a way of knowing the
value of OpenVPN internal mechansim.

This is required only if the GUI keeps "hold on" between SIGUSR1 restarts --- windows GUI does "hold off" before
hold release so hold will happen only the first time after a SIGHUP.

Quoted 12 lines

---
 doc/management-notes.txt |  7 +++++--
 src/openvpn/init.c       | 17 ++++++++++-------
 src/openvpn/manage.c     |  8 ++++++--
 src/openvpn/manage.h     |  2 +-
 4 files changed, 22 insertions(+), 12 deletions(-)

doc/management-notes.txt +0 -0

index f68f3db..eb857ab 100644

doc/management-notes.txt be reset by restarts. · line 175

Quoted 9 lines

doc/management-notes.txt +4 -1

@@ -168,9 +168,12 @@ be reset by restarts.
 
 OpenVPN will indicate that it is in a hold state by
 sending a real-time notification to the management
-client:
+client, the parameter indicates how long OpenVPN would
+wait without UI (as influenced connect-retry exponential
+backoff. The UI need to wait for releasing the hold if it
+wants similar behavior:

FWIW, some typos:

influenced -> influenced by
need --> needs
and a missing closing parenthesis?

doc/management-notes.txt be reset by restarts. · line 177

Quoted 4 lines

doc/management-notes.txt +1 -1

-  >HOLD:Waiting for hold release
+  >HOLD:Waiting for hold release:10

Windows GUI is fine with this, but potentially a UI may break with this change depending on how it parses the line. Just a warning to UI maintainers..

Rest of the code looks good.

Selva

Arne Schwabe

author

2016-10-12 10:47 UTC #2 lore raw

PATCH v2 1/1 Change the hold command to communicate the time that OpenVPN would wait to the UI.

Patch V2: Fixed typos noticed by Selva(

---
 doc/management-notes.txt |  7 +++++--
 src/openvpn/init.c       | 15 +++++++++------
 src/openvpn/manage.c     |  8 ++++++--
 src/openvpn/manage.h     |  2 +-
 4 files changed, 21 insertions(+), 11 deletions(-)

doc/management-notes.txt +5 -2

index f68f3db..dd870eb 100644
@@ -168,9 +168,12 @@ be reset by restarts.
 
 OpenVPN will indicate that it is in a hold state by
 sending a real-time notification to the management
-client:
+client, the parameter indicates how long OpenVPN would
+wait without UI (as influenced by connect-retry exponential
+backoff). The UI needs to wait for releasing the hold if it
+wants similar behavior:
 
-  >HOLD:Waiting for hold release
+  >HOLD:Waiting for hold release:10
 
 Command examples:

src/openvpn/init.c +9 -6

index 7684a96..73f8c6d 100644
@@ -1954,16 +1954,17 @@ do_deferred_options (struct context *c, const unsigned int found)
 }
 
 /*
- * Possible hold on initialization
+ * Possible hold on initialization, holdtime is the
+ * time OpenVPN would wait without management
  */
 static bool
-do_hold (void)
+do_hold (int holdtime)
 {
 #ifdef ENABLE_MANAGEMENT
   if (management)
     {
       /* block until management hold is released */
-      if (management_hold (management))
+        if (management_hold (management, holdtime))
 	return true;
     }
 #endif

@@ -2021,8 +2022,10 @@ socket_restart_pause (struct context *c)
   c->persist.restart_sleep_seconds = 0;
 
   /* do managment hold on context restart, i.e. second, third, fourth, etc. initialization */
-  if (do_hold ())
+  if (do_hold (sec))
+  {
     sec = 0;
+  }
 
   if (sec)
     {

@@ -2040,7 +2043,7 @@ do_startup_pause (struct context *c)
   if (!c->first_time)
     socket_restart_pause (c);
   else
-    do_hold (); /* do management hold on first context initialization */
+    do_hold (0); /* do management hold on first context initialization */
 }
 
 /*

@@ -3425,7 +3428,7 @@ open_management (struct context *c)
 	    }
 
 	  /* initial management hold, called early, before first context initialization */
-	  do_hold ();
+	  do_hold (0);
 	  if (IS_SIG (c))
 	    {
 	      msg (M_WARN, "Signal received from management interface, exiting");

src/openvpn/manage.c +6 -2

index dcb1bc1..206aaf4 100644
@@ -3332,12 +3332,13 @@ management_should_daemonize (struct management *man)
  * Return true if the caller should not sleep for an additional time interval.
  */
 bool
-management_hold (struct management *man)
+management_hold (struct management *man, int holdtime)
 {
   if (management_would_hold (man))
     {
       volatile int signal_received = 0;
       const bool standalone_disabled_save = man->persist.standalone_disabled;
+      struct gc_arena gc = gc_new ();
 
       man->persist.standalone_disabled = false; /* This is so M_CLIENT messages will be correctly passed through msg() */
       man->persist.special_state_msg = NULL;

@@ -3347,7 +3348,9 @@ management_hold (struct management *man)
 
       if (!signal_received)
 	{
-	  man->persist.special_state_msg = ">HOLD:Waiting for hold release";
+      struct buffer out = alloc_buf_gc (128, &gc);
+      buf_printf (&out, ">HOLD:Waiting for hold release:%d", holdtime);
+	  man->persist.special_state_msg = BSTR (&out);
 	  msg (M_CLIENT, "%s", man->persist.special_state_msg);
 
 	  /* run command processing event loop until we get our username/password */

@@ -3366,6 +3369,7 @@ management_hold (struct management *man)
       man->persist.special_state_msg = NULL;
       man->settings.mansig &= ~MANSIG_IGNORE_USR1_HUP;
 
+      gc_free (&gc);
       return true;
     }
   return false;

src/openvpn/manage.h +1 -1

index 988600f..50db38c 100644
@@ -396,7 +396,7 @@ int managment_android_persisttun_action (struct management *man);
 
 bool management_should_daemonize (struct management *man);
 bool management_would_hold (struct management *man);
-bool management_hold (struct management *man);
+bool management_hold (struct management *man, int holdtime);
 
 void management_event_loop_n_seconds (struct management *man, int sec);

Signature · 1 line

2.8.4 (Apple Git-73)

Selva Nair

2016-10-13 02:46 UTC #3 lore raw

Hi,

Quoted 8 lines · Arne Schwabe

Before the connect-retry change to do exponential backup this was not necessary
since the time was fixed. With the exponential backoff the UI needs either to
implement its own exponential backoff mechansim or needs a way of knowing the
value of OpenVPN internal mechansim.

Quoted 7 lines

Patch V2: Fixed typos noticed by Selva(

---
 doc/management-notes.txt |  7 +++++--
 src/openvpn/init.c       | 15 +++++++++------
 src/openvpn/manage.c     |  8 ++++++--
 src/openvpn/manage.h     |  2 +-
 4 files changed, 21 insertions(+), 11 deletions(-)

Looks good and works as expected. Tested on Linux.

- >HOLD:Waiting for hold release

Quoted 1 line

+ >HOLD:Waiting for hold release:10

OpenVPN GUI does not parse the "Waiting for hold release" string, so this doesn't affect it.

ACK from me.

Suggest to correct mechansim->mechanism (x2) in commit message during merge.

Selva

Gert Doering

2016-10-13 06:32 UTC #4 lore raw

Hi,

Quoted 2 lines · Selva Nair

Suggest to correct mechansim->mechanism (x2) in commit message during merge.

Will do. (Too busy today, tomorrow or weekend-ish)

Thanks for the review.

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]...

David Sommerseth

2016-10-13 16:17 UTC #5 lore raw

In reply to Arne Schwabe

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Your patch has been applied to the master branch

commit 396d30c264e6cb6b9f57c3e566f3b71879999662

Author: Arne Schwabe
Date:   Wed Oct 12 12:47:07 2016 +0200

     Change the hold command to communicate the time that OpenVPN would wait to the UI.

     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12675.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJX/7OFAAoJEIbPlEyWcf3yy8MQAKWMd5Y9NkUY36Visbkufu2R 89tEQakX26zInduOAmmf879FvZcUBRSRWoxY9H0Jacvik0h9IWIikfopzqCG/17A zMVS0XRcjylQf7OVrM8oSUCXTWoI15J+DibuI9PhyZUthBd3NZbH61c7RgCX9fzN dwFlEl9dYNj01WUtj5R3bFUwSUZl5kw1dua7pqYjJIIIYNAdzTCd8ML/VSTG0O+M 9iybH28d4jOcd154qomjka2Yqz5sSs4cK7gGZ9dDaIoMreI+gKIBQiM1nl6hz16z 2yg7B+4VOxsj9prhCGrFeQyzXO6H8cRc8+NyFIO8ZiVjDaSkdXNn6oixfnDulo+i go6Q5oPqs5wpk20b0KHIqBuz3fJ5DJobURRvazY3r7x3vFoYHg7Yo6f5VZyx+Xjh c19MIWzCBnQLrx7d/E3PsBXMBgfcElVwmM8+j0eaC1KB0wWSqnIwlDvUyjrPVNgA sFZQeTB664bKhxCkoH5vSd/pbevNm539HoMLmYIxJ8PwPuIcsWC+q7d2MkqBRage jxz1w73b2+dcTyYj91241QV2wY/UcaOFOSEafVwoloRc2vZeWxzx7dEDxYTtkVvO K3I8EY1hW3JsQzmGrRwIeJQ0dyEwlQWoIpoP1ra2roFYJcs+txTS2hS/E613v+U+ FrhurTzaG+gUTxM/EZTJ
=yszQ
-----END PGP SIGNATURE-----