[Openvpn-devel,v2,1/3] Move restoring pre pull options to initialising of c2 context

Message ID 20210317160038.25828-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v2,1/3] Move restoring pre pull options to initialising of c2 context
Related show

Commit Message

Arne Schwabe March 17, 2021, 4 p.m.
We currently delay restoring these options until we actually must
restore them. Since there is no reason to do so apart from the very
minor saving to not have to execute that code when a connection fails,
move them it into the general context_2 initialisation.

Patch V2: rebase on master.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/init.c    | 5 +++++
 src/openvpn/openvpn.h | 1 -
 src/openvpn/push.c    | 5 -----
 3 files changed, 5 insertions(+), 6 deletions(-)

Comments

Antonio Quartulli March 17, 2021, 4:30 p.m. | #1
Hi all,

On 17/03/2021 17:00, Arne Schwabe wrote:
> We currently delay restoring these options until we actually must
> restore them. Since there is no reason to do so apart from the very
> minor saving to not have to execute that code when a connection fails,
> move them it into the general context_2 initialisation.
> 
> Patch V2: rebase on master.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

This patch is simply about moving when the pre-pull settings have to be
restored. Aims at simplifying the code and make it easier to follow
(thanks to the patches that will come later).

This patch was reviewed with the Arne's guidance.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering March 23, 2021, 9:31 a.m. | #2
Your patch has been applied to the master branch.

I do not really have a test rig to test this, currently.  I think it would
need a long-running client instance that connects to a server, receives
a set of options, disconnects (server kicking it out?), reconnects, and
receives *different* options.  Or possibly "not receives options that
were set before", so verify "fall back to default/config file settings".

OTOH staring at the code looks reasonable - we only restore if something
was saved before (o->pre_pull initialized) and if we succeed connecting, 
it's the same amount of energy spent.

I have run this through the enhanced client side tests (including the
"must fail!") tests, but since these all only do single-shot connections,
this is not really excercising the code change.


I do wonder if connection *failures* would cause an increased memory
consumption now... is "&c->c2.gc" cleared eventually?  (Because we 
allocate new copies of route-lists in that GC)... *looking*... ah, yes,
c2.gc lives only for "one instance" (init_instance() to close_instance()),
and since that one talks about "advance connection entry", it looks
like "this dance is done per connection attempt"...

commit 528a78fb144ff6a3d5865c871a402ba98fdfe21e
Author: Arne Schwabe
Date:   Wed Mar 17 17:00:36 2021 +0100

     Move restoring pre pull options to initialising of c2 context

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210317160038.25828-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21676.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index d234729c..81aaa6c9 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -4165,6 +4165,11 @@  init_instance(struct context *c, const struct env_set *env, const unsigned int f
         }
     }
 
+    if (c->options.pull)
+    {
+        pre_pull_restore(&c->options, &c->c2.gc);
+    }
+
     /* map in current connection entry */
     next_connection_entry(c);
 
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index e9bc7dad..436c10ee 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -463,7 +463,6 @@  struct context_2
 
     struct event_timeout push_request_interval;
     time_t push_request_timeout;
-    bool did_pre_pull_restore;
 
     /* hash of pulled options, so we can compare when options change */
     bool pulled_options_digest_init_done;
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 320ad737..580c16bd 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -929,11 +929,6 @@  process_incoming_push_reply(struct context *c,
             md_ctx_init(c->c2.pulled_options_state, md_kt_get("SHA256"));
             c->c2.pulled_options_digest_init_done = true;
         }
-        if (!c->c2.did_pre_pull_restore)
-        {
-            pre_pull_restore(&c->options, &c->c2.gc);
-            c->c2.did_pre_pull_restore = true;
-        }
         if (apply_push_options(&c->options,
                                buf,
                                permission_mask,