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