| 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 |
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
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,
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); > } > } >
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>
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
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); } }