diff mbox series

[Openvpn-devel,2/2] Abort client-connect handler loop after first handler sets 'disable'.

Message ID 20200727183436.6625-2-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,1/2] Fix sequence of events for async plugin v1 handler. | expand

Commit Message

Gert Doering July 27, 2020, 8:34 a.m. UTC 
The old code would run all (succeeding) handlers, then discover "one of
them set the 'disable' flag for this client", and then unroll all the
handlers.

Moving the 'disable' check into the loop makes it stop after the first
handler that fails or (succeeds and sets 'disable').  This is a bit
more logical in the log files, and has less potential side effects
due to running "later" client-connect handlers when we already know
they will have to be unrolled.

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/multi.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Arne Schwabe July 27, 2020, 11:05 a.m. UTC | #1 
Am 27.07.20 um 20:34 schrieb Gert Doering:
> The old code would run all (succeeding) handlers, then discover "one of
> them set the 'disable' flag for this client", and then unroll all the
> handlers.
> 
> Moving the 'disable' check into the loop makes it stop after the first
> handler that fails or (succeeds and sets 'disable').  This is a bit
> more logical in the log files, and has less potential side effects
> due to running "later" client-connect handlers when we already know
> they will have to be unrolled.
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>

This another of the 'review is easy if you ignore white space' patches.
This slightly changes the behaviour but I don't see a problem with the
behaviour change to abort early.

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering July 27, 2020, 9:46 p.m. UTC | #2 
Patch has been applied to the master branch.

commit 20b394746a7a351d892bb8c21beb66dd138631d9
Author: Gert Doering
Date:   Mon Jul 27 20:34:36 2020 +0200

     Abort client-connect handler loop after first handler sets 'disable'.

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20200727183436.6625-2-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20612.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index cfb34720..0f9c586b 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2617,18 +2617,18 @@  multi_connection_established(struct multi_context *m, struct multi_instance *mi)
                 ASSERT(0);
         }
 
-        (*cur_handler_index)++;
-    }
+        /*
+         * Check for "disable" directive in client-config-dir file
+         * or config file generated by --client-connect script.
+         */
+        if (mi->context.options.disable)
+        {
+            msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to "
+                "'disable' directive");
+            cc_succeeded = false;
+        }
 
-    /*
-     * Check for "disable" directive in client-config-dir file
-     * or config file generated by --client-connect script.
-     */
-    if (mi->context.options.disable)
-    {
-        msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to "
-            "'disable' directive");
-        cc_succeeded = false;
+        (*cur_handler_index)++;
     }
 
     if (cc_succeeded)