diff mbox series

[Openvpn-devel,v4,02/13] client-connect: Refactor multi_client_connect_source_ccd

Message ID 20181121101019.1801-3-arne@rfc2549.org
State Superseded
Delegated to: Antonio Quartulli
Headers show
Series Deferred client-connect patch set | expand

Commit Message

Arne Schwabe Nov. 20, 2018, 11:10 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>
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/multi.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

Antonio Quartulli Dec. 3, 2019, 2:48 a.m. UTC | #1 
Hi,

On 21/11/2018 11:10, 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>
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/multi.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index a4b62151..8fc69a07 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2063,31 +2063,30 @@ multi_client_connect_source_ccd(struct multi_context *m,
>                                       &gc);
>  
>          /* try common-name file */
> -        if (platform_test_file(ccd_file))
> +        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);
> +            ccd_file = NULL;
>          }
> -        else /* try default file */
> +        /* try default file */

I don't get the change above.

You are removing the "else" but now what's the reason for testing
"platform_test_file()" and setting ccd_file to NULL if ccd_file is
getting re-assigned just below ?


it seems like this is a buggy behviaour and it is not matching what was
happening before the refactoring.

NAK.

>          {
>              ccd_file = platform_gen_path(mi->context.options.client_config_dir,
>                                           CCD_DEFAULT,
>                                           &gc);
> -
> -            if (platform_test_file(ccd_file))
> +            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);
> +                ccd_file = NULL;
>              }
>          }
> +
> +        if (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);
>      }
>  }
>
diff mbox series

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a4b62151..8fc69a07 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2063,31 +2063,30 @@  multi_client_connect_source_ccd(struct multi_context *m,
                                      &gc);
 
         /* try common-name file */
-        if (platform_test_file(ccd_file))
+        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);
+            ccd_file = NULL;
         }
-        else /* try default file */
+        /* try default file */
         {
             ccd_file = platform_gen_path(mi->context.options.client_config_dir,
                                          CCD_DEFAULT,
                                          &gc);
-
-            if (platform_test_file(ccd_file))
+            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);
+                ccd_file = NULL;
             }
         }
+
+        if (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);
     }
 }