diff mbox series

[Openvpn-devel,8/8] Code cleanup: remove superflous variable

Message ID 20200709101603.11941-8-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel,1/8] Deprecate ncp-disable and add improved ncp to Changes.rst |

Commit Message

Arne Schwabe July 9, 2020, 12:16 a.m. UTC 
  Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Antonio Quartulli July 9, 2020, 9:22 a.m. UTC | #1 
Hi,

On 09/07/2020 12:16, Arne Schwabe wrote:
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/ssl.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 4ee4c245..54a23011 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1231,11 +1231,10 @@ lame_duck_must_die(const struct tls_session *session, interval_t *wakeup)
>      const struct key_state *lame = &session->key[KS_LAME_DUCK];
>      if (lame->state >= S_INITIAL)
>      {
> -        const time_t local_now = now;
>          ASSERT(lame->must_die); /* a lame duck key must always have an expiration */
> -        if (local_now < lame->must_die)
> +        if (now < lame->must_die)
>          {
> -            compute_earliest_wakeup(wakeup, lame->must_die - local_now);
> +            compute_earliest_wakeup(wakeup, lame->must_die - now);

The only reason for having this local variable is the case where this
code would run concurrently with a thread that could update "now" behind
our back.

Since openvpn runs in a single thread, such scenario is not possible at all.

For this reason this patch makes sense and it removes one more bit that
was originally introduced in the attempt of implementing multi threading.

Acked-by: Antonio Quartulli <a@unstable.cc>

>              return false;
>          }
>          else
>
Gert Doering July 9, 2020, 9:26 a.m. UTC | #2 
Your patch has been applied to the master branch.

After some discussion on IRC, Antonio and I have come to the conclusion
that the original code was related to the pre-2.2 threading attempts, and
that "copying 'now' to a new local variable" might have been necessary to
avoid another thread messing with it while comparisons are being made - so
having a thread-local "local_now" is reasonable.

Now, the threading stuff was never finished and removed in 7aa6c12a4424d
nearly 10 years ago...  out with it :-)

commit ca514800ca126b69dbde846db819d8bf4c490e68
Author: Arne Schwabe
Date:   Thu Jul 9 12:16:03 2020 +0200

     Code cleanup: remove superflous variable

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


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 4ee4c245..54a23011 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1231,11 +1231,10 @@  lame_duck_must_die(const struct tls_session *session, interval_t *wakeup)
     const struct key_state *lame = &session->key[KS_LAME_DUCK];
     if (lame->state >= S_INITIAL)
     {
-        const time_t local_now = now;
         ASSERT(lame->must_die); /* a lame duck key must always have an expiration */
-        if (local_now < lame->must_die)
+        if (now < lame->must_die)
         {
-            compute_earliest_wakeup(wakeup, lame->must_die - local_now);
+            compute_earliest_wakeup(wakeup, lame->must_die - now);
             return false;
         }
         else