| Message ID | 20171202134541.7688-7-a@unstable.cc |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel,1/7] Remove option to disable crypto engine | expand |
Hi, On 02-12-17 14:45, Antonio Quartulli wrote: > Now that ENABLE_CRYPTO has been removed, TLS_MODE is basically > a useless shortcut which does not really help the readability of the > code. So I don't really agree here. As we've just experienced while discussing this on IRC, the meaning of 'c->c2.tls_multi != NULL' is not that trivial. At first sight it seems to suggest 'we have multiple peers' (i.e. mode server), but it really does just mean 'this is a TLS mode connection' (ie, not a static key connection). So even though I would have preferred this to be an inline function, I think it *does* add to readability. So I'd prefer to either keep it, or change it to a static inline function for type safety. > Remove it and use its expanded expression instead. > > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > src/openvpn/forward.c | 2 +- > src/openvpn/init.c | 2 +- > src/openvpn/occ.c | 3 ++- > src/openvpn/openvpn.h | 1 - > 4 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c > index 9bf9483e..85ec71ae 100644 > --- a/src/openvpn/forward.c > +++ b/src/openvpn/forward.c > @@ -948,7 +948,7 @@ process_incoming_link_part2(struct context *c, struct link_socket_info *lsi, con > * > * Also, update the persisted version of our packet-id. > */ > - if (!TLS_MODE(c)) > + if (!c->c2.tls_multi) > { > link_socket_set_outgoing_addr(&c->c2.buf, lsi, &c->c2.from, NULL, c->c2.es); > } > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index f8034ec7..7fe50628 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -1306,7 +1306,7 @@ do_init_timers(struct context *c, bool deferred) > /* initialize occ timers */ > > if (c->options.occ > - && !TLS_MODE(c) > + && !c->c2.tls_multi > && c->c2.options_string_local && c->c2.options_string_remote) > { > event_timeout_init(&c->c2.occ_interval, OCC_INTERVAL_SECONDS, now); > diff --git a/src/openvpn/occ.c b/src/openvpn/occ.c > index 40f7e768..8434920c 100644 > --- a/src/openvpn/occ.c > +++ b/src/openvpn/occ.c > @@ -378,7 +378,8 @@ process_received_occ_msg(struct context *c) > > case OCC_REPLY: > dmsg(D_PACKET_CONTENT, "RECEIVED OCC_REPLY"); > - if (c->options.occ && !TLS_MODE(c) && c->c2.options_string_remote) > + if (c->options.occ && !c->c2.tls_multi > + && c->c2.options_string_remote) > { > if (!options_cmp_equal_safe((char *) BPTR(&c->c2.buf), > c->c2.options_string_remote, > diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h > index fe8324ab..b36ca319 100644 > --- a/src/openvpn/openvpn.h > +++ b/src/openvpn/openvpn.h > @@ -556,7 +556,6 @@ struct context > * have been compiled in. > */ > > -#define TLS_MODE(c) ((c)->c2.tls_multi != NULL) > #define PROTO_DUMP_FLAGS (check_debug_level(D_LINK_RW_VERBOSE) ? (PD_SHOW_DATA|PD_VERBOSE) : 0) > #define PROTO_DUMP(buf, gc) protocol_dump((buf), \ > PROTO_DUMP_FLAGS \ > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 03/12/17 22:45, Steffan Karger wrote: > Hi, > > On 02-12-17 14:45, Antonio Quartulli wrote: >> Now that ENABLE_CRYPTO has been removed, TLS_MODE is basically >> a useless shortcut which does not really help the readability of the >> code. > > So I don't really agree here. As we've just experienced while > discussing this on IRC, the meaning of 'c->c2.tls_multi != NULL' is not > that trivial. At first sight it seems to suggest 'we have multiple > peers' (i.e. mode server), but it really does just mean 'this is a TLS > mode connection' (ie, not a static key connection). > > So even though I would have preferred this to be an inline function, I > think it *does* add to readability. So I'd prefer to either keep it, or > change it to a static inline function for type safety. Let's drop this patch then. With another patch we can convert all these macros to inline static functions. Cheers, > >> Remove it and use its expanded expression instead. >> >> Signed-off-by: Antonio Quartulli <a@unstable.cc> >> --- >> src/openvpn/forward.c | 2 +- >> src/openvpn/init.c | 2 +- >> src/openvpn/occ.c | 3 ++- >> src/openvpn/openvpn.h | 1 - >> 4 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c >> index 9bf9483e..85ec71ae 100644 >> --- a/src/openvpn/forward.c >> +++ b/src/openvpn/forward.c >> @@ -948,7 +948,7 @@ process_incoming_link_part2(struct context *c, struct link_socket_info *lsi, con >> * >> * Also, update the persisted version of our packet-id. >> */ >> - if (!TLS_MODE(c)) >> + if (!c->c2.tls_multi) >> { >> link_socket_set_outgoing_addr(&c->c2.buf, lsi, &c->c2.from, NULL, c->c2.es); >> } >> diff --git a/src/openvpn/init.c b/src/openvpn/init.c >> index f8034ec7..7fe50628 100644 >> --- a/src/openvpn/init.c >> +++ b/src/openvpn/init.c >> @@ -1306,7 +1306,7 @@ do_init_timers(struct context *c, bool deferred) >> /* initialize occ timers */ >> >> if (c->options.occ >> - && !TLS_MODE(c) >> + && !c->c2.tls_multi >> && c->c2.options_string_local && c->c2.options_string_remote) >> { >> event_timeout_init(&c->c2.occ_interval, OCC_INTERVAL_SECONDS, now); >> diff --git a/src/openvpn/occ.c b/src/openvpn/occ.c >> index 40f7e768..8434920c 100644 >> --- a/src/openvpn/occ.c >> +++ b/src/openvpn/occ.c >> @@ -378,7 +378,8 @@ process_received_occ_msg(struct context *c) >> >> case OCC_REPLY: >> dmsg(D_PACKET_CONTENT, "RECEIVED OCC_REPLY"); >> - if (c->options.occ && !TLS_MODE(c) && c->c2.options_string_remote) >> + if (c->options.occ && !c->c2.tls_multi >> + && c->c2.options_string_remote) >> { >> if (!options_cmp_equal_safe((char *) BPTR(&c->c2.buf), >> c->c2.options_string_remote, >> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h >> index fe8324ab..b36ca319 100644 >> --- a/src/openvpn/openvpn.h >> +++ b/src/openvpn/openvpn.h >> @@ -556,7 +556,6 @@ struct context >> * have been compiled in. >> */ >> >> -#define TLS_MODE(c) ((c)->c2.tls_multi != NULL) >> #define PROTO_DUMP_FLAGS (check_debug_level(D_LINK_RW_VERBOSE) ? (PD_SHOW_DATA|PD_VERBOSE) : 0) >> #define PROTO_DUMP(buf, gc) protocol_dump((buf), \ >> PROTO_DUMP_FLAGS \ >> > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel >
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 9bf9483e..85ec71ae 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -948,7 +948,7 @@ process_incoming_link_part2(struct context *c, struct link_socket_info *lsi, con * * Also, update the persisted version of our packet-id. */ - if (!TLS_MODE(c)) + if (!c->c2.tls_multi) { link_socket_set_outgoing_addr(&c->c2.buf, lsi, &c->c2.from, NULL, c->c2.es); } diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f8034ec7..7fe50628 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1306,7 +1306,7 @@ do_init_timers(struct context *c, bool deferred) /* initialize occ timers */ if (c->options.occ - && !TLS_MODE(c) + && !c->c2.tls_multi && c->c2.options_string_local && c->c2.options_string_remote) { event_timeout_init(&c->c2.occ_interval, OCC_INTERVAL_SECONDS, now); diff --git a/src/openvpn/occ.c b/src/openvpn/occ.c index 40f7e768..8434920c 100644 --- a/src/openvpn/occ.c +++ b/src/openvpn/occ.c @@ -378,7 +378,8 @@ process_received_occ_msg(struct context *c) case OCC_REPLY: dmsg(D_PACKET_CONTENT, "RECEIVED OCC_REPLY"); - if (c->options.occ && !TLS_MODE(c) && c->c2.options_string_remote) + if (c->options.occ && !c->c2.tls_multi + && c->c2.options_string_remote) { if (!options_cmp_equal_safe((char *) BPTR(&c->c2.buf), c->c2.options_string_remote, diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h index fe8324ab..b36ca319 100644 --- a/src/openvpn/openvpn.h +++ b/src/openvpn/openvpn.h @@ -556,7 +556,6 @@ struct context * have been compiled in. */ -#define TLS_MODE(c) ((c)->c2.tls_multi != NULL) #define PROTO_DUMP_FLAGS (check_debug_level(D_LINK_RW_VERBOSE) ? (PD_SHOW_DATA|PD_VERBOSE) : 0) #define PROTO_DUMP(buf, gc) protocol_dump((buf), \ PROTO_DUMP_FLAGS \
Now that ENABLE_CRYPTO has been removed, TLS_MODE is basically a useless shortcut which does not really help the readability of the code. Remove it and use its expanded expression instead. Signed-off-by: Antonio Quartulli <a@unstable.cc> --- src/openvpn/forward.c | 2 +- src/openvpn/init.c | 2 +- src/openvpn/occ.c | 3 ++- src/openvpn/openvpn.h | 1 - 4 files changed, 4 insertions(+), 4 deletions(-)