[Openvpn-devel,v5,06/14] client-connect: Refactor client-connect handling to calling a bunch of hooks in a loop

Message ID 20200711093655.23686-6-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v5,01/14] Allow changing fallback cipher from ccd files/client-connect
Related show

Commit Message

Arne Schwabe July 11, 2020, 9:36 a.m.
From: Fabian Knittel <fabian.knittel@lettink.de>

This patch changes the calling of the client-connect functions into an array
of hooks and a block of code that calls them in a loop.

Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Patch V5: Rebase on master.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/multi.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

Comments

Gert Doering July 13, 2020, 1:23 p.m. | #1
Hi,

On Sat, Jul 11, 2020 at 11:36:47AM +0200, Arne Schwabe wrote:
> From: Fabian Knittel <fabian.knittel@lettink.de>
> 
> This patch changes the calling of the client-connect functions into an array
> of hooks and a block of code that calls them in a loop.
> 
> Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

23...
Test sets succeeded: 1 1a 1b 1d 2 2a 2b 2c 2d 3 4 5 6 8 8a 9.
Test sets failed: none.
24...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 4a 5 6 8 8a 9.
Test sets failed: none.
master...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 5 6 7 7a 8 8a 9 2f 4b.
Test sets failed: none.


I dot not test all variants yet, though, so this is not a sufficient
test to say "everything works".

Generally speaking the patch looks quite reasonable.

gert
Antonio Quartulli July 14, 2020, 9:55 a.m. | #2
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> From: Fabian Knittel <fabian.knittel@lettink.de>
> 
> This patch changes the calling of the client-connect functions into an array
> of hooks and a block of code that calls them in a loop.
> 
> Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> 
> Patch V5: Rebase on master.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/multi.c | 43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 9bb52ef7..83848fdc 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2241,6 +2241,10 @@ cc_check_return(int *cc_succeeded_count,
>      }
>  }
>  
> +typedef enum client_connect_return (*multi_client_connect_handler)
> +    (struct multi_context *m, struct multi_instance *mi,
> +     unsigned int *option_types_found);
> +

how about something like this:

2519 typedef enum client_connect_return
2520 (*multi_client_connect_handler)(struct multi_context *m,
2521                                 struct multi_instance *mi,
2522                                 unsigned int *option_types_found);
2523

I find it easier to read (just my opinion)


>  /*
>   * Called as soon as the SSL/TLS connection is authenticated.
>   *
> @@ -2268,7 +2272,17 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
>      {
>          return;
>      }
> -    unsigned int option_types_found = 0;
> +
> +        multi_client_connect_handler handlers[] = {
> +            multi_client_connect_source_ccd,
> +            multi_client_connect_call_plugin_v1,
> +            multi_client_connect_call_plugin_v2,
> +            multi_client_connect_call_script,
> +            multi_client_connect_mda,
> +            NULL
> +        };
> +
> +        unsigned int option_types_found = 0;

something is wrong in the indentation of the chunk above: too many spaces.

>  
>      int cc_succeeded = true; /* client connect script status */
>      int cc_succeeded_count = 0;
> @@ -2276,32 +2290,9 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
>  
>      multi_client_connect_early_setup(m, mi);
>  
> -    ret = multi_client_connect_source_ccd(m, mi, &option_types_found);
> -    cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
> -
> -    if (cc_succeeded)
> -    {
> -        ret = multi_client_connect_call_plugin_v1(m, mi, &option_types_found);
> -        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
> -    }
> -
> -    if (cc_succeeded)
> -    {
> -        ret = multi_client_connect_call_plugin_v2(m, mi, &option_types_found);
> -        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
> -    }
> -
> -
> -    if (cc_succeeded)
> +    for (int i = 0; cc_succeeded && handlers[i]; i++)
>      {
> -        ret = multi_client_connect_call_script(m, mi, &option_types_found);
> -        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
> -    }
> -
> -    if (cc_succeeded)
> -    {
> -
> -        ret = multi_client_connect_mda(m, mi, &option_types_found);
> +        ret = handlers[i](m, mi, &option_types_found);
>          cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
>      }
>  
> 


Except for the indentation issue, the rest looks good. This patch simply
makes the handlers invocation more generic and part of a loop.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering July 16, 2020, 6:06 p.m. | #3
Your patch has been applied to the master branch.

I have *not* reformatted the multi_client_connect_handler block - this is 
stuff that goes away in 08/14 again, so reformatting now is doubly futile.

(Speaking of patch granularity: the combination of 05+06+08 is causing 
three times work for something which is basically one change - as the result 
of 05 goes away in 06, and 06 gets rewritten in 08 again.  This makes 
review and testing significantly more work than just doing the change)

commit 07a69fd2ca934c0521b0ee25f72e7f2385279a71
Author: Fabian Knittel
Date:   Sat Jul 11 11:36:47 2020 +0200

     client-connect: Refactor client-connect handling to calling a bunch of hooks in a loop

     Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <a@unstable.cc>
     Message-Id: <20200711093655.23686-6-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20293.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9bb52ef7..83848fdc 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2241,6 +2241,10 @@  cc_check_return(int *cc_succeeded_count,
     }
 }
 
+typedef enum client_connect_return (*multi_client_connect_handler)
+    (struct multi_context *m, struct multi_instance *mi,
+     unsigned int *option_types_found);
+
 /*
  * Called as soon as the SSL/TLS connection is authenticated.
  *
@@ -2268,7 +2272,17 @@  multi_connection_established(struct multi_context *m, struct multi_instance *mi)
     {
         return;
     }
-    unsigned int option_types_found = 0;
+
+        multi_client_connect_handler handlers[] = {
+            multi_client_connect_source_ccd,
+            multi_client_connect_call_plugin_v1,
+            multi_client_connect_call_plugin_v2,
+            multi_client_connect_call_script,
+            multi_client_connect_mda,
+            NULL
+        };
+
+        unsigned int option_types_found = 0;
 
     int cc_succeeded = true; /* client connect script status */
     int cc_succeeded_count = 0;
@@ -2276,32 +2290,9 @@  multi_connection_established(struct multi_context *m, struct multi_instance *mi)
 
     multi_client_connect_early_setup(m, mi);
 
-    ret = multi_client_connect_source_ccd(m, mi, &option_types_found);
-    cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-
-    if (cc_succeeded)
-    {
-        ret = multi_client_connect_call_plugin_v1(m, mi, &option_types_found);
-        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-    }
-
-    if (cc_succeeded)
-    {
-        ret = multi_client_connect_call_plugin_v2(m, mi, &option_types_found);
-        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-    }
-
-
-    if (cc_succeeded)
+    for (int i = 0; cc_succeeded && handlers[i]; i++)
     {
-        ret = multi_client_connect_call_script(m, mi, &option_types_found);
-        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-    }
-
-    if (cc_succeeded)
-    {
-
-        ret = multi_client_connect_mda(m, mi, &option_types_found);
+        ret = handlers[i](m, mi, &option_types_found);
         cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
     }