[Openvpn-devel,5/7] Remove CIPHER_ENABLED

Message ID 20171202134541.7688-5-a@unstable.cc
State Rejected
Headers show
Series
  • [Openvpn-devel,1/7] Remove option to disable crypto engine
Related show

Commit Message

Antonio Quartulli Dec. 2, 2017, 1:45 p.m.
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(-)

Comments

Steffan Karger Dec. 3, 2017, 2:17 p.m. | #1
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
Gert Doering Dec. 4, 2017, 6:45 p.m. | #2
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
Antonio Quartulli Dec. 5, 2017, 6:45 a.m. | #3
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,

Patch

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