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