[Openvpn-devel,v5,03/14] client-connect: Refactor multi_client_connect_source_ccd

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

Commit Message

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

Refactor multi_client_connect_source_ccd(), so that options_server_import() (or
the success path in general) is only entered in one place within the function.

Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>

Patch V5: Simplify the logic even further to make more easy to understand.

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

Comments

Gert Doering July 13, 2020, 1:29 a.m. UTC | #1
Hi,

On Sat, Jul 11, 2020 at 11:36:44AM +0200, Arne Schwabe wrote:
> From: Fabian Knittel <fabian.knittel@lettink.de>
> 
> Refactor multi_client_connect_source_ccd(), so that options_server_import() (or
> the success path in general) is only entered in one place within the function.
> 
> Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>

All server side tests passed

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.

which is NOT a conclusive test in this case, because I do not have 
clients setup with ccd/DEFAULT here.  So this is more "it does not break
anything else".


I do not like the indentation and wrapping, though:

> +        const char *ccd_client = platform_gen_path(mi->context.options.client_config_dir,
> +                                                   tls_common_name(mi->context.c2.tls_multi,
> +                                                                   false),
> +                                                   &gc);

instead, maybe this?

> +        const char *ccd_client = 
> +             platform_gen_path(mi->context.options.client_config_dir,
> +                               tls_common_name(mi->context.c2.tls_multi,
> +                                               false), &gc);

or maybe we want to extract "tls_common_name(..., false)" into a temp
variable here? 

> +        const char *cn = tls_common_name(mi->context.c2.tls_multi, false);
> +        const char *ccd_client = 
> +             platform_gen_path(mi->context.options.client_config_dir, 
                                  cn, &gc);


gert
Antonio Quartulli July 13, 2020, 2:16 a.m. UTC | #2
Hi,

On 13/07/2020 13:29, Gert Doering wrote:
> instead, maybe this?
> 
>> +        const char *ccd_client = 
>> +             platform_gen_path(mi->context.options.client_config_dir,
>> +                               tls_common_name(mi->context.c2.tls_multi,
>> +                                               false), &gc);
> 

I also like the above.

> or maybe we want to extract "tls_common_name(..., false)" into a temp
> variable here? 
> 
>> +        const char *cn = tls_common_name(mi->context.c2.tls_multi, false);
>> +        const char *ccd_client = 
>> +             platform_gen_path(mi->context.options.client_config_dir, 
>                                   cn, &gc);
> 

Imho this is not prettier than the version above :D so I'd personally go
with the version above instead of this.


Cheers,
tincanteksup July 13, 2020, 2:50 a.m. UTC | #3
1x grammar

On 11/07/2020 10:36, Arne Schwabe wrote:
> From: Fabian Knittel <fabian.knittel@lettink.de>
> 
> Refactor multi_client_connect_source_ccd(), so that options_server_import() (or
> the success path in general) is only entered in one place within the function.
> 
> Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
> 
> Patch V5: Simplify the logic even further to make more easy to understand.

grammar: to make it more easy


> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   src/openvpn/multi.c | 43 +++++++++++++++++++++----------------------
>   1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 3c4ceeb5..35e0bd10 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2164,15 +2164,30 @@ multi_client_connect_source_ccd(struct multi_context *m,
>       if (mi->context.options.client_config_dir)
>       {
>           struct gc_arena gc = gc_new();
> -        const char *ccd_file;
> +        const char *ccd_file = NULL;
> +
> +        const char *ccd_client = platform_gen_path(mi->context.options.client_config_dir,
> +                                                   tls_common_name(mi->context.c2.tls_multi,
> +                                                                   false),
> +                                                   &gc);
> +
> +        const char *ccd_default = platform_gen_path(mi->context.options.client_config_dir,
> +                                                    CCD_DEFAULT,
> +                                                    &gc);
>   
> -        ccd_file = platform_gen_path(mi->context.options.client_config_dir,
> -                                     tls_common_name(mi->context.c2.tls_multi,
> -                                                     false),
> -                                     &gc);
>   
>           /* try common-name file */
> -        if (platform_test_file(ccd_file))
> +        if (platform_test_file(ccd_client))
> +        {
> +            ccd_file = ccd_client;
> +        }
> +        /* try default file */
> +        else if (platform_test_file(ccd_default))
> +        {
> +            ccd_file = ccd_default;
> +        }
> +
> +        if (ccd_file)
>           {
>               options_server_import(&mi->context.options,
>                                     ccd_file,
> @@ -2181,22 +2196,6 @@ multi_client_connect_source_ccd(struct multi_context *m,
>                                     option_types_found,
>                                     mi->context.c2.es);
>           }
> -        else /* try default file */
> -        {
> -            ccd_file = platform_gen_path(mi->context.options.client_config_dir,
> -                                         CCD_DEFAULT,
> -                                         &gc);
> -
> -            if (platform_test_file(ccd_file))
> -            {
> -                options_server_import(&mi->context.options,
> -                                      ccd_file,
> -                                      D_IMPORT_ERRORS|M_OPTERR,
> -                                      CLIENT_CONNECT_OPT_MASK,
> -                                      option_types_found,
> -                                      mi->context.c2.es);
> -            }
> -        }
>           gc_free(&gc);
>       }
>   }
>
Antonio Quartulli July 13, 2020, 10:51 p.m. UTC | #4
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> From: Fabian Knittel <fabian.knittel@lettink.de>
> 
> Refactor multi_client_connect_source_ccd(), so that options_server_import() (or
> the success path in general) is only entered in one place within the function.
> 
> Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
> 
> Patch V5: Simplify the logic even further to make more easy to understand.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---

Shouldn't this patch also bear Fabian's signature?

Anyway, other than the stylistic point made by Gert this patch looks
good. It's mostly creating a new interface without really introducing
any radical change.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering July 14, 2020, 3:26 a.m. UTC | #5
Your patch has been applied to the master branch.

Tested on the server side framework (though I still do not have a ccd/DEFAULT
file set up...) - it's not breaking anything unexpected, and the change "should"
not break DEFAULT.

Rewrapped as discussed on the list (... I have the itch to go through all the
source and rewrap all these "long assignments and the function arguments
line up in the last 20 characters of each line"... but that's a different
patch)

commit 62a840e2ab3096ded56971a1fa789fe22896ba35
Author: Fabian Knittel
Date:   Sat Jul 11 11:36:44 2020 +0200

     client-connect: Refactor multi_client_connect_source_ccd

     Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20200711093655.23686-3-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20287.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 3c4ceeb5..35e0bd10 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2164,15 +2164,30 @@  multi_client_connect_source_ccd(struct multi_context *m,
     if (mi->context.options.client_config_dir)
     {
         struct gc_arena gc = gc_new();
-        const char *ccd_file;
+        const char *ccd_file = NULL;
+
+        const char *ccd_client = platform_gen_path(mi->context.options.client_config_dir,
+                                                   tls_common_name(mi->context.c2.tls_multi,
+                                                                   false),
+                                                   &gc);
+
+        const char *ccd_default = platform_gen_path(mi->context.options.client_config_dir,
+                                                    CCD_DEFAULT,
+                                                    &gc);
 
-        ccd_file = platform_gen_path(mi->context.options.client_config_dir,
-                                     tls_common_name(mi->context.c2.tls_multi,
-                                                     false),
-                                     &gc);
 
         /* try common-name file */
-        if (platform_test_file(ccd_file))
+        if (platform_test_file(ccd_client))
+        {
+            ccd_file = ccd_client;
+        }
+        /* try default file */
+        else if (platform_test_file(ccd_default))
+        {
+            ccd_file = ccd_default;
+        }
+
+        if (ccd_file)
         {
             options_server_import(&mi->context.options,
                                   ccd_file,
@@ -2181,22 +2196,6 @@  multi_client_connect_source_ccd(struct multi_context *m,
                                   option_types_found,
                                   mi->context.c2.es);
         }
-        else /* try default file */
-        {
-            ccd_file = platform_gen_path(mi->context.options.client_config_dir,
-                                         CCD_DEFAULT,
-                                         &gc);
-
-            if (platform_test_file(ccd_file))
-            {
-                options_server_import(&mi->context.options,
-                                      ccd_file,
-                                      D_IMPORT_ERRORS|M_OPTERR,
-                                      CLIENT_CONNECT_OPT_MASK,
-                                      option_types_found,
-                                      mi->context.c2.es);
-            }
-        }
         gc_free(&gc);
     }
 }