Re: [Openvpn-devel] [PATCH] Handle ctrl-C and ctrl-break events on Windows

[Gert Doering]

Hi,

On Sun, Nov 22, 2015 at 10:49:13AM -0500, Selva Nair wrote:
> > (Is it actually quicker as well, so does the nssm timeout still need to
> > be adjusted?  Haven't seen feedback from Samuli here yet)
> >
> 
> In my tests openvpn exit processing takes only a few hundred msec, but if
> exit-notify is enabled that adds about 1 to 2 seconds[*]. As that option
> could be pushed as well its safer to setup nssm with a delay of  2500 msec
> between ctrl-C and 'kill without prejudice' (aka Terminate).

Ah, yes.  Thanks :-)  (you and Samuli need to work on the NSSM setup stuff
then, I don't have an eye on the windows stuff, so won't be able to track
this so it's not getting lost)

> [*} actually with exit-notify n, it should only take n-1 seconds to send n
> notifies, but integer arithmetic makes it always a little more than n-1.
> And for some reason it appears to wait a second even if another notify
> doesn't have to be sent. I've not looked into that part of the code
> carefully...

Our timer granularity is "1 second" for most stuff, and we tend to add
another second "just to be sure"... ran into this when trying to speed
up client PUSH_REQUEST sending in commit afb93fac803f

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Handle ctrl-C and ctrl-break events on Windows

[Selva Nair]

Hi,

On Sun, Nov 22, 2015 at 10:18 AM, Gert Doering  wrote:

> Hi,
>
> On Mon, Nov 16, 2015 at 09:48:09PM -0500, Selva Nair wrote:
> > v2 changes
> >  - cleaner, hopefully easier to get a code review :)
> >  - handles both console mode and service mode
> > -- >8 --
> >
> > Handle ctrl-C or ctrl-Break sent to the console as a SIGTERM.
> > Depending on the console mode, windows delivers ctrl-C as a
> > keyboard input or as a signal. We handle both cases. This allows
> > graceful termination of openvpn from programs such as nssm.
> > Works in both console mode and service mode.
>
> If I read this right, the new code uses WriteConsoleInput() only to
> signal "hey, some work to do here, wake up!" without actually sending any
> keypress-to-itself, and the signal itself is sent by throw_signal()?
>

Yes, that's the idea.

This is certainly nicer :-)
>

Agree :)


>
> (Is it actually quicker as well, so does the nssm timeout still need to
> be adjusted?  Haven't seen feedback from Samuli here yet)
>

In my tests openvpn exit processing takes only a few hundred msec, but if
exit-notify is enabled that adds about 1 to 2 seconds[*]. As that option
could be pushed as well its safer to setup nssm with a delay of  2500 msec
between ctrl-C and 'kill without prejudice' (aka Terminate).

Thanks,

Selva

[*} actually with exit-notify n, it should only take n-1 seconds to send n
notifies, but integer arithmetic makes it always a little more than n-1.
And for some reason it appears to wait a second even if another notify
doesn't have to be sent. I've not looked into that part of the code
carefully...

Re: [Openvpn-devel] [PATCH] Handle ctrl-C and ctrl-break events on Windows

[Gert Doering]

Hi,

On Mon, Nov 16, 2015 at 09:48:09PM -0500, Selva Nair wrote:
> v2 changes
>  - cleaner, hopefully easier to get a code review :)
>  - handles both console mode and service mode
> -- >8 --
> 
> Handle ctrl-C or ctrl-Break sent to the console as a SIGTERM.
> Depending on the console mode, windows delivers ctrl-C as a
> keyboard input or as a signal. We handle both cases. This allows
> graceful termination of openvpn from programs such as nssm.
> Works in both console mode and service mode.

If I read this right, the new code uses WriteConsoleInput() only to
signal "hey, some work to do here, wake up!" without actually sending any
keypress-to-itself, and the signal itself is sent by throw_signal()?

This is certainly nicer :-)

(Is it actually quicker as well, so does the nssm timeout still need to
be adjusted?  Haven't seen feedback from Samuli here yet)

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

[Openvpn-devel] [PATCH] Handle ctrl-C and ctrl-break events on Windows

[Selva Nair]

v2 changes
 - cleaner, hopefully easier to get a code review :)
 - handles both console mode and service mode
-- >8 --

Handle ctrl-C or ctrl-Break sent to the console as a SIGTERM.
Depending on the console mode, windows delivers ctrl-C as a
keyboard input or as a signal. We handle both cases. This allows
graceful termination of openvpn from programs such as nssm.
Works in both console mode and service mode.

Signed-off-by: Selva Nair 
---
 src/openvpn/win32.c | 53 +
 1 file changed, 52 insertions(+)

diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index d06b41f..1f9bda0 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -324,6 +324,53 @@ net_event_win32_close (struct net_event_win32 *ne)
  * (2) Service mode -- map Windows event object to SIGTERM
  */

+static void
+win_trigger_event(struct win32_signal *ws)
+{
+  if (ws->mode == WSO_MODE_SERVICE && HANDLE_DEFINED(ws->in.read))
+SetEvent (ws->in.read);
+  else /* generate a key-press event */
+{
+  DWORD tmp;
+  INPUT_RECORD ir;
+  HANDLE stdin_handle = GetStdHandle(STD_INPUT_HANDLE);
+
+  CLEAR(ir);
+  ir.EventType = KEY_EVENT;
+  ir.Event.KeyEvent.bKeyDown = true;
+  if (!stdin_handle || !WriteConsoleInput(stdin_handle, , 1, ))
+msg(M_WARN|M_ERRNO, "WARN: win_trigger_event: WriteConsoleInput");
+}
+}
+
+/*
+ * Callback to handle console ctrl events
+ */
+static bool WINAPI
+win_ctrl_handler (DWORD signum)
+{
+  msg(D_LOW, "win_ctrl_handler: signal received (code=%lu)", (unsigned long) 
signum);
+
+  if (siginfo_static.signal_received == SIGTERM)
+ return true;
+
+  switch (signum)
+{
+case CTRL_C_EVENT:
+case CTRL_BREAK_EVENT:
+  throw_signal(SIGTERM);
+  /* trigget the win32_signal to interrupt the event loop */
+  win_trigger_event(_signal);
+  return true;
+  break;
+default:
+  msg(D_LOW, "win_ctrl_handler: signal (code=%lu) not handled", (unsigned 
long) signum);
+  break;
+}
+  /* pass all other signals to the next handler */
+  return false;
+}
+
 void
 win32_signal_clear (struct win32_signal *ws)
 {
@@ -403,6 +450,9 @@ win32_signal_open (struct win32_signal *ws,
ws->mode = WSO_MODE_SERVICE;
}
 }
+/* set the ctrl handler in both console and service modes */
+if (!SetConsoleCtrlHandler ((PHANDLER_ROUTINE) win_ctrl_handler, true))
+   msg (M_WARN|M_ERRNO, "WARN: SetConsoleCtrlHandler failed");
 }

 static bool
@@ -512,6 +562,9 @@ win32_signal_get (struct win32_signal *ws)
case 0x3E: /* F4 -> TERM */
  ret = SIGTERM;
  break;
+   case 0x03: /* CTRL-C -> TERM */
+ ret = SIGTERM;
+ break;
}
}
   if (ret)
-- 
2.6.2

Re: [Openvpn-devel] [PATCH] Handle ctrl-C and ctrl-break events on Windows

[Selva Nair]

Hi,

On Fri, Nov 13, 2015 at 2:36 PM, Gert Doering  wrote:
> On Wed, Nov 11, 2015 at 02:46:10PM -0500, Selva Nair wrote:
> [..]
>> Tested on windows 7 with cmd-line use and start/stop with nssm. For nssm, 
>> the default
>> delay after ctrl-C is 1500 msec which is not enough for the process to exit 
>> if exit-notify
>> is being used. About 1 second per extit notify trials + an extra 1 second is 
>> suggested.
>
> So this would be something for Samuli to adjust when setting up NSSM?  Like,
> 2500 ms?

That is correct. It could be added to the nssm info page that Samuli
maintains. I use 3000 msec with "--explicit-exit-notify 2" and works well.

Selva

Re: [Openvpn-devel] [PATCH] Handle ctrl-C and ctrl-break events on Windows

[Gert Doering]

Hi,

On Wed, Nov 11, 2015 at 02:46:10PM -0500, Selva Nair wrote:
> > With nssm, the console is shared with nssm, so ctrl-C is delivered as
> > a signal. I'll send a patch handling both cases.
> 
> The patch is in the next email. 
> 
> Handling the key-board input is easy, but making the callback interrupt the 
> event loop
> looks tricky. I am not well versed with the event handling in the code, so 
> opted to generate a key-pressed event. If there is a better way please 
> suggest.

Arne might have an idea here... I do not understand the event handling
that well either.

[..]
> Tested on windows 7 with cmd-line use and start/stop with nssm. For nssm, the 
> default 
> delay after ctrl-C is 1500 msec which is not enough for the process to exit 
> if exit-notify
> is being used. About 1 second per extit notify trials + an extra 1 second is 
> suggested. 

So this would be something for Samuli to adjust when setting up NSSM?  Like,
2500 ms?

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

[Openvpn-devel] [PATCH] Handle ctrl-C and ctrl-break events on Windows

[Selva Nair]

On Mon, Nov 9, 2015 at 2:03 PM, Selva Nair  wrote:
>> It's probably okay to just make CTRL-c generate a SIGTERM as F4 is
>> already doing.
>>
>> James
>
> Thanks for the comment.
>
> In the interactive mode, the console is opened with no
> ENABLE_PROCESSED_INPUT so ctrl-C will be delivered as key-board input
> and could be handled just like F4.
>
> With nssm, the console is shared with nssm, so ctrl-C is delivered as
> a signal. I'll send a patch handling both cases.

The patch is in the next email. 

Handling the key-board input is easy, but making the callback interrupt the 
event loop
looks tricky. I am not well versed with the event handling in the code, so 
opted to generate a key-pressed event. If there is a better way please suggest.

Simply calling throw_signal in the callback will work, but could take several 
seconds
before its noticed.

Tested on windows 7 with cmd-line use and start/stop with nssm. For nssm, the 
default 
delay after ctrl-C is 1500 msec which is not enough for the process to exit if 
exit-notify
is being used. About 1 second per extit notify trials + an extra 1 second is 
suggested. 

 src/openvpn/win32.c | 54 +
 1 file changed, 54 insertions(+)

-- 
2.6.2