| Message ID | 20200711093655.23686-4-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v5,01/14] Allow changing fallback cipher from ccd files/client-connect | expand |
On Sat, Jul 11, 2020 at 11:36:45AM +0200, Arne Schwabe wrote: > From: Fabian Knittel <fabian.knittel@lettink.de> > > This patch moves multi_client_connect_setenv into > multi_client_connect_early_setup and makes sure that every client-connect > handling function updates the virtual address selection. > > Background: This unifies how the client-connect handling functions work. Tested on the server side framework, all tests succeed 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. The patch itself looks a bit strange, now code seems to be duplicated... ... hopefully this gets better in the next patches. gert
Hi, On 11/07/2020 11:36, Arne Schwabe wrote: > From: Fabian Knittel <fabian.knittel@lettink.de> > > This patch moves multi_client_connect_setenv into > multi_client_connect_early_setup and makes sure that every client-connect > handling function updates the virtual address selection. > > Background: This unifies how the client-connect handling functions work. > > 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 | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index 35e0bd10..539ebfc0 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2150,6 +2150,12 @@ multi_client_connect_early_setup(struct multi_context *m, > > /* reset pool handle to null */ > mi->vaddr_handle = -1; > + > + /* do --client-connect setenvs */ > + multi_select_virtual_addr(m, mi); > + > + multi_client_connect_setenv(m, mi); > + > } > > /** > @@ -2195,6 +2201,13 @@ multi_client_connect_source_ccd(struct multi_context *m, > CLIENT_CONNECT_OPT_MASK, > option_types_found, > mi->context.c2.es); > + /* > + * Select a virtual address from either --ifconfig-push in > + * --client-config-dir file or --ifconfig-pool. > + */ > + multi_select_virtual_addr(m, mi); > + > + multi_client_connect_setenv(m, mi); > } > gc_free(&gc); > } With this patch applied we are moving these 2 lines to: - multi_client_connect_source_ccd() - multi_client_connect_early_setup() In multi_connection_established() we call the two aforementioned functions one after the other. This means we are really executing the select_virtual_addr() twice in a row. Am I wrong? Maybe this gets fixed in a later patch, but are we sure we are not breaking anything? If this double call gets rearranged in a later patch...maybe it's better to merge these 2 patches? Bisect may become quite ugly otherwise. Cheers, > @@ -2236,15 +2249,6 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi) > > multi_client_connect_source_ccd(m, mi, &option_types_found); > > - /* > - * Select a virtual address from either --ifconfig-push in > - * --client-config-dir file or --ifconfig-pool. > - */ > - multi_select_virtual_addr(m, mi); > - > - /* do --client-connect setenvs */ > - multi_client_connect_setenv(m, mi); > - > multi_client_connect_call_plugin_v1(m, mi, &option_types_found, > &cc_succeeded, > &cc_succeeded_count); >
Acked-by: Gert Doering <gert@greenie.muc.de> Your patch has been applied to the master branch. It has been stared-at, and tested on the server side test rig. We've had quite a bit of discussion about this on IRC, and the conclusion is "multiple calls to these functions might be needed in some situations" (like, when a ccd/ file sets up a virtual address and a plugin or script wants to act on the assigned IP address, by means of $ENV{ifconfig_pool_local} etc.) - we might want to make it more clever ("if nothing changes, do not free/reallocate stuff all the time"), or at least join them all in one place with a clear comment 13:10 <@cron2> moving this to multi_client_connect_early_setup() seems wrong 13:10 <@cron2> we should move it to the very end, after all "bring in new config" bits have been concluded 13:11 <@plaisthos> this might want to have a connect script act on the assigned ip 13:11 <@plaisthos> like dynamic routing etc 13:11 <@plaisthos> and if you move it to the end then you loose the environment variable with the assigned IP 13:12 <@cron2> in that case, you actually need to really call it after every single "could alter config" call 13:12 <@cron2> and before the first 13:12 <@plaisthos> technically you don't need it before the first 13:12 <@plaisthos> because the first is ccd 13:12 <@plaisthos> that does not evaluate env 13:12 <@cron2> true 13:13 <@plaisthos> but for consistency and the later refactoring it is better to do it anyway I guess 13:14 <@cron2> okay. I think 04/ can go in, then, but after the change to "just walk the table of handlers" I'd move all calls to both multi_select_virtual_addr() and multi_client_connect_setenv() into this loop - so we know "it's evalued after each handler, because further handles might need the environment" - and then we can kick it from early_setup() again 13:14 <@cron2> the stuff is all reentrant, so 04/ is not the most elegant, but it is not doing harm commit 380a142a6b397e615ef809a94c5e6be7fcddad06 Author: Fabian Knittel Date: Sat Jul 11 11:36:45 2020 +0200 client-connect: Move multi_client_connect_setenv into early_setup 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: Gert Doering <gert@greenie.muc.de> Message-Id: <20200711093655.23686-4-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20288.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 35e0bd10..539ebfc0 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2150,6 +2150,12 @@ multi_client_connect_early_setup(struct multi_context *m, /* reset pool handle to null */ mi->vaddr_handle = -1; + + /* do --client-connect setenvs */ + multi_select_virtual_addr(m, mi); + + multi_client_connect_setenv(m, mi); + } /** @@ -2195,6 +2201,13 @@ multi_client_connect_source_ccd(struct multi_context *m, CLIENT_CONNECT_OPT_MASK, option_types_found, mi->context.c2.es); + /* + * Select a virtual address from either --ifconfig-push in + * --client-config-dir file or --ifconfig-pool. + */ + multi_select_virtual_addr(m, mi); + + multi_client_connect_setenv(m, mi); } gc_free(&gc); } @@ -2236,15 +2249,6 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi) multi_client_connect_source_ccd(m, mi, &option_types_found); - /* - * Select a virtual address from either --ifconfig-push in - * --client-config-dir file or --ifconfig-pool. - */ - multi_select_virtual_addr(m, mi); - - /* do --client-connect setenvs */ - multi_client_connect_setenv(m, mi); - multi_client_connect_call_plugin_v1(m, mi, &option_types_found, &cc_succeeded, &cc_succeeded_count);