| Message ID | 20171202134541.7688-5-a@unstable.cc |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel,1/7] Remove option to disable crypto engine | expand |
On 02-12-17 14:45, Antonio Quartulli wrote: > Now that ENABLE_CRYPTO has been removed, CIPHER_ENABLED 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/init.c | 4 ++-- > src/openvpn/openvpn.h | 2 -- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index e013e9ca..f8034ec7 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -2379,7 +2379,7 @@ frame_finalize_options(struct context *c, const struct options *o) > * Set adjustment factor for buffer alignment when no > * cipher is used. > */ > - if (!CIPHER_ENABLED(c)) > + if (!c->c1.ks.key_type.cipher) > { > frame_align_to_extra_frame(&c->c2.frame); > frame_or_align_flags(&c->c2.frame, > @@ -2904,7 +2904,7 @@ do_init_frame(struct context *c) > * flexible enough for this, since the frame is already established > * before it is known which compression options will be pushed. > */ > - if (comp_unswapped_prefix(&c->options.comp) && CIPHER_ENABLED(c)) > + if (comp_unswapped_prefix(&c->options.comp) && c->c1.ks.key_type.cipher) > { > frame_add_to_align_adjust(&c->c2.frame, COMP_PREFIX_LEN); > frame_or_align_flags(&c->c2.frame, > diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h > index fb8ff1a4..d843c913 100644 > --- a/src/openvpn/openvpn.h > +++ b/src/openvpn/openvpn.h > @@ -565,8 +565,6 @@ struct context > gc) > #define MD5SUM(buf, len, gc) md5sum((buf), (len), 0, (gc)) > > -#define CIPHER_ENABLED(c) (c->c1.ks.key_type.cipher != NULL) > - > /* this represents "disabled peer-id" */ > #define MAX_PEER_ID 0xFFFFFF > Acked-by: Steffan Karger <steffan@karger.me> -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Sun, Dec 03, 2017 at 03:17:51PM +0100, Steffan Karger wrote: > On 02-12-17 14:45, Antonio Quartulli wrote: > > Now that ENABLE_CRYPTO has been removed, CIPHER_ENABLED is basically > > a useless shortcut which does not really help the readability of the > > code. Call me silly, but CIPHER_ENABLED has nothing to do with ENABLE_CRYPTO, but more with "--cipher none", no? So at least the commit message is misleading... > > * Set adjustment factor for buffer alignment when no > > * cipher is used. > > */ > > - if (!CIPHER_ENABLED(c)) > > + if (!c->c1.ks.key_type.cipher) ... and I don't really find the second one more easily understandable than the first one... gert
Hi, On 05/12/17 02:45, Gert Doering wrote: > Hi, > > On Sun, Dec 03, 2017 at 03:17:51PM +0100, Steffan Karger wrote: >> On 02-12-17 14:45, Antonio Quartulli wrote: >>> Now that ENABLE_CRYPTO has been removed, CIPHER_ENABLED is basically >>> a useless shortcut which does not really help the readability of the >>> code. > > Call me silly, but CIPHER_ENABLED has nothing to do with ENABLE_CRYPTO, > but more with "--cipher none", no? > "Remove ENABLE_CRYPTO" introduced this change: -#ifdef ENABLE_CRYPTO #define CIPHER_ENABLED(c) (c->c1.ks.key_type.cipher != NULL) -#else -#define CIPHER_ENABLED(c) (false) -#endif thus my feeling was that this macro is now not so useful, because there is no conditional definition anymore. > So at least the commit message is misleading... > >>> * Set adjustment factor for buffer alignment when no >>> * cipher is used. >>> */ >>> - if (!CIPHER_ENABLED(c)) >>> + if (!c->c1.ks.key_type.cipher) > > ... and I don't really find the second one more easily understandable > than the first one... > I think it's a matter of taste :-) We could substitute every NULL check with a human readable macro/inline-functions, but I am not sure it would be really useful. Honestly, this condition seem quite clear to me (at least compared to the TLS_MODE()/tls_multi one). But, again, this is just my taste: feel free to drop this patch if you think it is better to keep the macro. Cheers,
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index e013e9ca..f8034ec7 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2379,7 +2379,7 @@ frame_finalize_options(struct context *c, const struct options *o) * Set adjustment factor for buffer alignment when no * cipher is used. */ - if (!CIPHER_ENABLED(c)) + if (!c->c1.ks.key_type.cipher) { frame_align_to_extra_frame(&c->c2.frame); frame_or_align_flags(&c->c2.frame, @@ -2904,7 +2904,7 @@ do_init_frame(struct context *c) * flexible enough for this, since the frame is already established * before it is known which compression options will be pushed. */ - if (comp_unswapped_prefix(&c->options.comp) && CIPHER_ENABLED(c)) + if (comp_unswapped_prefix(&c->options.comp) && c->c1.ks.key_type.cipher) { frame_add_to_align_adjust(&c->c2.frame, COMP_PREFIX_LEN); frame_or_align_flags(&c->c2.frame, diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h index fb8ff1a4..d843c913 100644 --- a/src/openvpn/openvpn.h +++ b/src/openvpn/openvpn.h @@ -565,8 +565,6 @@ struct context gc) #define MD5SUM(buf, len, gc) md5sum((buf), (len), 0, (gc)) -#define CIPHER_ENABLED(c) (c->c1.ks.key_type.cipher != NULL) - /* this represents "disabled peer-id" */ #define MAX_PEER_ID 0xFFFFFF
Now that ENABLE_CRYPTO has been removed, CIPHER_ENABLED 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/init.c | 4 ++-- src/openvpn/openvpn.h | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-)