[Openvpn-devel,3/8] Extract process_incoming_push_reply from process_incoming_push_msg

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

Commit Message

Arne Schwabe July 9, 2020, 10:15 a.m.
This is a small refactoring to make both function more readable. It also
eliminates the ret variable in process_incoming_push_msg that now serves
no purpose anymore.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/push.c | 113 +++++++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 49 deletions(-)

Comments

Antonio Quartulli July 9, 2020, 3:42 p.m. | #1
Hi,

another nice patch that makes one more step towards having a more
readable code base!

On 09/07/2020 12:15, Arne Schwabe wrote:
> This is a small refactoring to make both function more readable. It also
> eliminates the ret variable in process_incoming_push_msg that now serves
> no purpose anymore.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/push.c | 113 +++++++++++++++++++++++++--------------------
>  1 file changed, 64 insertions(+), 49 deletions(-)
> 
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 56d652a3..d74323db 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -855,6 +855,63 @@ push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt)
>      }
>  }
>  
> +static int
> +process_incoming_push_reply(struct context *c,
> +                            unsigned int permission_mask,
> +                            const unsigned int *option_types_found,
> +                            struct buffer *buf)
> +{
> +    int ret = PUSH_MSG_ERROR;
> +    const uint8_t ch = buf_read_u8(buf);
> +    if (ch == ',')
> +    {
> +        struct buffer buf_orig = (*buf);
> +        if (!c->c2.pulled_options_digest_init_done)
> +        {
> +            c->c2.pulled_options_state = md_ctx_new();
> +            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,
> +                               option_types_found,
> +                               c->c2.es))
> +        {
> +            push_update_digest(c->c2.pulled_options_state, &buf_orig,
> +                               &c->options);
> +            switch (c->options.push_continuation)
> +            {
> +                case 0:
> +                case 1:
> +                    md_ctx_final(c->c2.pulled_options_state,
> +                                 c->c2.pulled_options_digest.digest);
> +                    md_ctx_cleanup(c->c2.pulled_options_state);
> +                    md_ctx_free(c->c2.pulled_options_state);
> +                    c->c2.pulled_options_state = NULL;
> +                    c->c2.pulled_options_digest_init_done = false;
> +                    ret = PUSH_MSG_REPLY;
> +                    break;
> +
> +                case 2:
> +                    ret = PUSH_MSG_CONTINUATION;
> +                    break;
> +            }
> +        }
> +    }
> +    else if (ch == '\0')
> +    {
> +        ret = PUSH_MSG_REPLY;
> +    }
> +    /* show_settings (&c->options); */
> +    return ret;
> +}
> +
>  int
>  process_incoming_push_msg(struct context *c,
>                            const struct buffer *buffer,
> @@ -862,64 +919,22 @@ process_incoming_push_msg(struct context *c,
>                            unsigned int permission_mask,
>                            unsigned int *option_types_found)

                             ^^^ this should now be made const,
otherwise you'll trigger a warning because process_incoming_push_reply()
wants const.

>  {
> -    int ret = PUSH_MSG_ERROR;
>      struct buffer buf = *buffer;
>  
>      if (buf_string_compare_advance(&buf, "PUSH_REQUEST"))
>      {
> -        ret = process_incoming_push_request(c);
> +        return process_incoming_push_request(c);
>      }
>      else if (honor_received_options
>               && buf_string_compare_advance(&buf, "PUSH_REPLY"))
>      {
> -        const uint8_t ch = buf_read_u8(&buf);
> -        if (ch == ',')
> -        {
> -            struct buffer buf_orig = buf;
> -            if (!c->c2.pulled_options_digest_init_done)
> -            {
> -                c->c2.pulled_options_state = md_ctx_new();
> -                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,
> -                                   option_types_found,
> -                                   c->c2.es))
> -            {
> -                push_update_digest(c->c2.pulled_options_state, &buf_orig,
> -                                   &c->options);
> -                switch (c->options.push_continuation)
> -                {
> -                    case 0:
> -                    case 1:
> -                        md_ctx_final(c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
> -                        md_ctx_cleanup(c->c2.pulled_options_state);
> -                        md_ctx_free(c->c2.pulled_options_state);
> -                        c->c2.pulled_options_state = NULL;
> -                        c->c2.pulled_options_digest_init_done = false;
> -                        ret = PUSH_MSG_REPLY;
> -                        break;
> -
> -                    case 2:
> -                        ret = PUSH_MSG_CONTINUATION;
> -                        break;
> -                }
> -            }
> -        }
> -        else if (ch == '\0')
> -        {
> -            ret = PUSH_MSG_REPLY;
> -        }
> -        /* show_settings (&c->options); */
> +        return process_incoming_push_reply(c, permission_mask,
> +                                           option_types_found, &buf);
> +    }
> +    else
> +    {
> +        return PUSH_MSG_ERROR;
>      }
> -    return ret;
>  }
>  
>  
> 

The rest looks good!
Tested on the client side and the PUSH_REPLY was still properly
processed as expected,


Assuming that const gets fixed:

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering July 9, 2020, 5 p.m. | #2
Your patch has been applied to the master branch.

After some discussion on IRC regarding the "const" bit it was decided
to not *add* it to the function call, but to *remove* it from the
function definition of process_incoming_push_msg()  (which would
otherwise be done in the 5/8 patch of this series).  

With that changed the code compiles without warnings.  

Stared a bit at "git show -w --color-moved=zebra" and the change seems 
to make sense :-)

Tested with local t_client tests on Linux and FreeBSD - haven't done 
a full server test, because "it's client side code".

commit 5e78bf66fa97818e0587ab1504cf7ecfd73df944
Author: Arne Schwabe
Date:   Thu Jul 9 12:15:58 2020 +0200

     Extract process_incoming_push_reply from process_incoming_push_msg

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 56d652a3..d74323db 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -855,6 +855,63 @@  push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt)
     }
 }
 
+static int
+process_incoming_push_reply(struct context *c,
+                            unsigned int permission_mask,
+                            const unsigned int *option_types_found,
+                            struct buffer *buf)
+{
+    int ret = PUSH_MSG_ERROR;
+    const uint8_t ch = buf_read_u8(buf);
+    if (ch == ',')
+    {
+        struct buffer buf_orig = (*buf);
+        if (!c->c2.pulled_options_digest_init_done)
+        {
+            c->c2.pulled_options_state = md_ctx_new();
+            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,
+                               option_types_found,
+                               c->c2.es))
+        {
+            push_update_digest(c->c2.pulled_options_state, &buf_orig,
+                               &c->options);
+            switch (c->options.push_continuation)
+            {
+                case 0:
+                case 1:
+                    md_ctx_final(c->c2.pulled_options_state,
+                                 c->c2.pulled_options_digest.digest);
+                    md_ctx_cleanup(c->c2.pulled_options_state);
+                    md_ctx_free(c->c2.pulled_options_state);
+                    c->c2.pulled_options_state = NULL;
+                    c->c2.pulled_options_digest_init_done = false;
+                    ret = PUSH_MSG_REPLY;
+                    break;
+
+                case 2:
+                    ret = PUSH_MSG_CONTINUATION;
+                    break;
+            }
+        }
+    }
+    else if (ch == '\0')
+    {
+        ret = PUSH_MSG_REPLY;
+    }
+    /* show_settings (&c->options); */
+    return ret;
+}
+
 int
 process_incoming_push_msg(struct context *c,
                           const struct buffer *buffer,
@@ -862,64 +919,22 @@  process_incoming_push_msg(struct context *c,
                           unsigned int permission_mask,
                           unsigned int *option_types_found)
 {
-    int ret = PUSH_MSG_ERROR;
     struct buffer buf = *buffer;
 
     if (buf_string_compare_advance(&buf, "PUSH_REQUEST"))
     {
-        ret = process_incoming_push_request(c);
+        return process_incoming_push_request(c);
     }
     else if (honor_received_options
              && buf_string_compare_advance(&buf, "PUSH_REPLY"))
     {
-        const uint8_t ch = buf_read_u8(&buf);
-        if (ch == ',')
-        {
-            struct buffer buf_orig = buf;
-            if (!c->c2.pulled_options_digest_init_done)
-            {
-                c->c2.pulled_options_state = md_ctx_new();
-                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,
-                                   option_types_found,
-                                   c->c2.es))
-            {
-                push_update_digest(c->c2.pulled_options_state, &buf_orig,
-                                   &c->options);
-                switch (c->options.push_continuation)
-                {
-                    case 0:
-                    case 1:
-                        md_ctx_final(c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
-                        md_ctx_cleanup(c->c2.pulled_options_state);
-                        md_ctx_free(c->c2.pulled_options_state);
-                        c->c2.pulled_options_state = NULL;
-                        c->c2.pulled_options_digest_init_done = false;
-                        ret = PUSH_MSG_REPLY;
-                        break;
-
-                    case 2:
-                        ret = PUSH_MSG_CONTINUATION;
-                        break;
-                }
-            }
-        }
-        else if (ch == '\0')
-        {
-            ret = PUSH_MSG_REPLY;
-        }
-        /* show_settings (&c->options); */
+        return process_incoming_push_reply(c, permission_mask,
+                                           option_types_found, &buf);
+    }
+    else
+    {
+        return PUSH_MSG_ERROR;
     }
-    return ret;
 }