diff mbox series

[Openvpn-devel] options: Always define options->management_flags

Message ID 20221127142506.41986-1-frank@lichtenheld.com
State Accepted
Headers show
Series [Openvpn-devel] options: Always define options->management_flags | expand

Commit Message

Frank Lichtenheld Nov. 27, 2022, 2:25 p.m. UTC 
That makes it possible to remove several preprocessor
directives which is a good thing. The cost should be
negligible.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
---
 src/openvpn/manage.h  | 41 +++++++++++++++++++-------------------
 src/openvpn/options.c | 46 +++++++++++++------------------------------
 src/openvpn/options.h |  6 ++++--
 3 files changed, 39 insertions(+), 54 deletions(-)

I tried to make options.c less ugly. I didn't get far. But it still
might be an incremental improvement.

Comments

Frank Lichtenheld Dec. 16, 2022, 1:11 p.m. UTC | #1 
On Mon, Dec 12, 2022 at 12:38:41PM +0100, Arne Schwabe wrote:
> Am 27.11.22 um 15:25 schrieb Frank Lichtenheld:
> > That makes it possible to remove several preprocessor
> > directives which is a good thing. The cost should be
> > negligible.
> 
> Acked-By: Arne Schwabe <arne@rfc2549.org>
> 
> In general the commit looks good but I would like to improve the comment for
> the management flags (see below).
> 
> 
> > diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> > index 68ad0cac..6f4b1f4a 100644
> > --- a/src/openvpn/options.h
> > +++ b/src/openvpn/options.h
> > @@ -438,10 +438,12 @@ struct options
> >       const char *management_client_user;
> >       const char *management_client_group;
> > -    /* Mask of MF_ values of manage.h */
> > -    unsigned int management_flags;
> >       const char *management_certificate;
> >   #endif
> > +    /* Mask of MF_ values of manage.h
> > +     * Always available to simplify options.c
> > +     */
> > +    unsigned int management_flags;
> 
> This feels a bit odd as a comment. I would expect this in the commit or
> something like "even available when management support is not available" but
> this comment feels a bit too "temporary".

How is this "temporary"? It does not describe the change, it actually states
why this is handled specially. Your proposal just drops the justification, then
the comment becomes useless IMHO.

Would you prefer "Always available to simplify option verification code"?

Regards,
Arne Schwabe Jan. 11, 2023, 11:02 a.m. UTC | #2 
Am 16.12.22 um 14:11 schrieb Frank Lichtenheld:
> On Mon, Dec 12, 2022 at 12:38:41PM +0100, Arne Schwabe wrote:
>> Am 27.11.22 um 15:25 schrieb Frank Lichtenheld:
>>> That makes it possible to remove several preprocessor
>>> directives which is a good thing. The cost should be
>>> negligible.
>>
>> Acked-By: Arne Schwabe <arne@rfc2549.org>
>>
>> In general the commit looks good but I would like to improve the comment for
>> the management flags (see below).
>>
>>
>>> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
>>> index 68ad0cac..6f4b1f4a 100644
>>> --- a/src/openvpn/options.h
>>> +++ b/src/openvpn/options.h
>>> @@ -438,10 +438,12 @@ struct options
>>>        const char *management_client_user;
>>>        const char *management_client_group;
>>> -    /* Mask of MF_ values of manage.h */
>>> -    unsigned int management_flags;
>>>        const char *management_certificate;
>>>    #endif
>>> +    /* Mask of MF_ values of manage.h
>>> +     * Always available to simplify options.c
>>> +     */
>>> +    unsigned int management_flags;
>>
>> This feels a bit odd as a comment. I would expect this in the commit or
>> something like "even available when management support is not available" but
>> this comment feels a bit too "temporary".
> 
> How is this "temporary"? It does not describe the change, it actually states
> why this is handled specially. Your proposal just drops the justification, then
> the comment becomes useless IMHO.
> 
> Would you prefer "Always available to simplify option verification code"?

I feel this because it is always available to avoid ifdefs and therefore 
to simplify options.c but the information that ifdefs are the 
alternative is not obvious from the comment or code alone. We have a lot 
of other variables that are always available but are only used if 
certain compile options are used. So I feel this comment is temporary 
because it basically is from the history that here in this case we had 
ifdefs, so we added here now a comment that there now no longer ifdefs. 
But in general without the historical context the comment feels odd to me.

Arne
Frank Lichtenheld Jan. 11, 2023, 12:25 p.m. UTC | #3 
On Wed, Jan 11, 2023 at 12:02:14PM +0100, Arne Schwabe wrote:
> Am 16.12.22 um 14:11 schrieb Frank Lichtenheld:
> > On Mon, Dec 12, 2022 at 12:38:41PM +0100, Arne Schwabe wrote:
> > > Am 27.11.22 um 15:25 schrieb Frank Lichtenheld:
> > > > That makes it possible to remove several preprocessor
> > > > directives which is a good thing. The cost should be
> > > > negligible.
> > > 
> > > Acked-By: Arne Schwabe <arne@rfc2549.org>
> > > 
> > > In general the commit looks good but I would like to improve the comment for
> > > the management flags (see below).
> > > 
> > > 
> > > > diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> > > > index 68ad0cac..6f4b1f4a 100644
> > > > --- a/src/openvpn/options.h
> > > > +++ b/src/openvpn/options.h
> > > > @@ -438,10 +438,12 @@ struct options
> > > >        const char *management_client_user;
> > > >        const char *management_client_group;
> > > > -    /* Mask of MF_ values of manage.h */
> > > > -    unsigned int management_flags;
> > > >        const char *management_certificate;
> > > >    #endif
> > > > +    /* Mask of MF_ values of manage.h
> > > > +     * Always available to simplify options.c
> > > > +     */
> > > > +    unsigned int management_flags;
> > > 
> > > This feels a bit odd as a comment. I would expect this in the commit or
> > > something like "even available when management support is not available" but
> > > this comment feels a bit too "temporary".
> > 
> > How is this "temporary"? It does not describe the change, it actually states
> > why this is handled specially. Your proposal just drops the justification, then
> > the comment becomes useless IMHO.
> > 
> > Would you prefer "Always available to simplify option verification code"?
> 
> I feel this because it is always available to avoid ifdefs and therefore to
> simplify options.c but the information that ifdefs are the alternative is
> not obvious from the comment or code alone. We have a lot of other variables
> that are always available but are only used if certain compile options are
> used. So I feel this comment is temporary because it basically is from the
> history that here in this case we had ifdefs, so we added here now a comment
> that there now no longer ifdefs. But in general without the historical
> context the comment feels odd to me.

I don't have a strong enough opinion about this to argue more about it.
Gert, feel free to take Arne's ack and just remove the additional comment line
I added.

Regards,
Gert Doering Jan. 11, 2023, 12:53 p.m. UTC | #4 
The ACK from Arne is not on the list, but it's in the quote from Frank,
so I can say "I have seen it" (and since the discussion went on about
a comment line, it's not a fake :-) ).  I have removed that comment
line per discussion on IRC.

Tested by compiling normally and with --disable-management, and asking
for a GHA test run.

Your patch has been applied to the master branch.

commit ff7d7989e007a8c66fba438a257c1b85e8bcca69 (master)
commit c8b131e5eb174a2c81a29a0b7eab5110e2e3ca19 (release/2.6)
Author: Frank Lichtenheld
Date:   Sun Nov 27 15:25:06 2022 +0100

     options: Always define options->management_flags

     Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20221127142506.41986-1-frank@lichtenheld.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25554.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index f46274e6..16ac6847 100644
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -24,6 +24,27 @@ 
 #ifndef MANAGE_H
 #define MANAGE_H
 
+/* management_open flags */
+#define MF_SERVER            (1<<0)
+#define MF_QUERY_PASSWORDS   (1<<1)
+#define MF_HOLD              (1<<2)
+#define MF_SIGNAL            (1<<3)
+#define MF_FORGET_DISCONNECT (1<<4)
+#define MF_CONNECT_AS_CLIENT (1<<5)
+#define MF_CLIENT_AUTH       (1<<6)
+/* #define MF_CLIENT_PF         (1<<7) *REMOVED FEATURE* */
+#define MF_UNIX_SOCK                (1<<8)
+#define MF_EXTERNAL_KEY             (1<<9)
+#define MF_EXTERNAL_KEY_NOPADDING   (1<<10)
+#define MF_EXTERNAL_KEY_PKCS1PAD    (1<<11)
+#define MF_UP_DOWN                  (1<<12)
+#define MF_QUERY_REMOTE             (1<<13)
+#define MF_QUERY_PROXY              (1<<14)
+#define MF_EXTERNAL_CERT            (1<<15)
+#define MF_EXTERNAL_KEY_PSSPAD      (1<<16)
+#define MF_EXTERNAL_KEY_DIGEST      (1<<17)
+
+
 #ifdef ENABLE_MANAGEMENT
 
 #include "misc.h"
@@ -321,26 +342,6 @@  struct user_pass;
 
 struct management *management_init(void);
 
-/* management_open flags */
-#define MF_SERVER            (1<<0)
-#define MF_QUERY_PASSWORDS   (1<<1)
-#define MF_HOLD              (1<<2)
-#define MF_SIGNAL            (1<<3)
-#define MF_FORGET_DISCONNECT (1<<4)
-#define MF_CONNECT_AS_CLIENT (1<<5)
-#define MF_CLIENT_AUTH       (1<<6)
-/* #define MF_CLIENT_PF         (1<<7) *REMOVED FEATURE* */
-#define MF_UNIX_SOCK                (1<<8)
-#define MF_EXTERNAL_KEY             (1<<9)
-#define MF_EXTERNAL_KEY_NOPADDING   (1<<10)
-#define MF_EXTERNAL_KEY_PKCS1PAD    (1<<11)
-#define MF_UP_DOWN                  (1<<12)
-#define MF_QUERY_REMOTE             (1<<13)
-#define MF_QUERY_PROXY              (1<<14)
-#define MF_EXTERNAL_CERT            (1<<15)
-#define MF_EXTERNAL_KEY_PSSPAD      (1<<16)
-#define MF_EXTERNAL_KEY_DIGEST      (1<<17)
-
 bool management_open(struct management *man,
                      const char *addr,
                      const char *port,
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index b7b34c9c..5eca4a39 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1952,24 +1952,24 @@  show_settings(const struct options *o)
     SHOW_STR_INLINE(ca_file);
     SHOW_STR(ca_path);
     SHOW_STR_INLINE(dh_file);
-#ifdef ENABLE_MANAGEMENT
     if ((o->management_flags & MF_EXTERNAL_CERT))
     {
         SHOW_PARM("cert_file", "EXTERNAL_CERT", "%s");
     }
     else
-#endif
-    SHOW_STR_INLINE(cert_file);
+    {
+        SHOW_STR_INLINE(cert_file);
+    }
     SHOW_STR_INLINE(extra_certs_file);
 
-#ifdef ENABLE_MANAGEMENT
     if ((o->management_flags & MF_EXTERNAL_KEY))
     {
         SHOW_PARM("priv_key_file", "EXTERNAL_PRIVATE_KEY", "%s");
     }
     else
-#endif
-    SHOW_STR_INLINE(priv_key_file);
+    {
+        SHOW_STR_INLINE(priv_key_file);
+    }
 #ifndef ENABLE_CRYPTO_MBEDTLS
     SHOW_STR_INLINE(pkcs12_file);
 #endif
@@ -2425,7 +2425,7 @@  options_postprocess_verify_ce(const struct options *options,
 
 #endif /* ifdef ENABLE_MANAGEMENT */
 
-#if defined(ENABLE_MANAGEMENT) && !defined(HAVE_XKEY_PROVIDER)
+#if !defined(HAVE_XKEY_PROVIDER)
     if ((tls_version_max() >= TLS_VER_1_3)
         && (options->management_flags & MF_EXTERNAL_KEY)
         && !(options->management_flags & (MF_EXTERNAL_KEY_NOPADDING))
@@ -2846,7 +2846,6 @@  options_postprocess_verify_ce(const struct options *options,
             {
                 msg(M_USAGE, "Parameter --key cannot be used when --pkcs11-provider is also specified.");
             }
-#ifdef ENABLE_MANAGEMENT
             if (options->management_flags & MF_EXTERNAL_KEY)
             {
                 msg(M_USAGE, "Parameter --management-external-key cannot be used when --pkcs11-provider is also specified.");
@@ -2855,7 +2854,6 @@  options_postprocess_verify_ce(const struct options *options,
             {
                 msg(M_USAGE, "Parameter --management-external-cert cannot be used when --pkcs11-provider is also specified.");
             }
-#endif
             if (options->pkcs12_file)
             {
                 msg(M_USAGE, "Parameter --pkcs12 cannot be used when --pkcs11-provider is also specified.");
@@ -2869,7 +2867,6 @@  options_postprocess_verify_ce(const struct options *options,
         }
         else
 #endif /* ifdef ENABLE_PKCS11 */
-#ifdef ENABLE_MANAGEMENT
         if ((options->management_flags & MF_EXTERNAL_KEY) && options->priv_key_file)
         {
             msg(M_USAGE, "--key and --management-external-key are mutually exclusive");
@@ -2886,7 +2883,6 @@  options_postprocess_verify_ce(const struct options *options,
             }
         }
         else
-#endif
 #ifdef ENABLE_CRYPTOAPI
         if (options->cryptoapi_cert)
         {
@@ -2902,7 +2898,6 @@  options_postprocess_verify_ce(const struct options *options,
             {
                 msg(M_USAGE, "Parameter --pkcs12 cannot be used when --cryptoapicert is also specified.");
             }
-#ifdef ENABLE_MANAGEMENT
             if (options->management_flags & MF_EXTERNAL_KEY)
             {
                 msg(M_USAGE, "Parameter --management-external-key cannot be used when --cryptoapicert is also specified.");
@@ -2911,7 +2906,6 @@  options_postprocess_verify_ce(const struct options *options,
             {
                 msg(M_USAGE, "Parameter --management-external-cert cannot be used when --cryptoapicert is also specified.");
             }
-#endif
         }
         else
 #endif /* ifdef ENABLE_CRYPTOAPI */
@@ -2932,7 +2926,6 @@  options_postprocess_verify_ce(const struct options *options,
             {
                 msg(M_USAGE, "Parameter --key cannot be used when --pkcs12 is also specified.");
             }
-#ifdef ENABLE_MANAGEMENT
             if (options->management_flags & MF_EXTERNAL_KEY)
             {
                 msg(M_USAGE, "Parameter --management-external-key cannot be used when --pkcs12 is also specified.");
@@ -2941,7 +2934,6 @@  options_postprocess_verify_ce(const struct options *options,
             {
                 msg(M_USAGE, "Parameter --management-external-cert cannot be used when --pkcs12 is also specified.");
             }
-#endif
 #endif /* ifdef ENABLE_CRYPTO_MBEDTLS */
         }
         else
@@ -2956,12 +2948,8 @@  options_postprocess_verify_ce(const struct options *options,
             {
 
                 const int sum =
-#ifdef ENABLE_MANAGEMENT
                     ((options->cert_file != NULL) || (options->management_flags & MF_EXTERNAL_CERT))
-                    +((options->priv_key_file != NULL) || (options->management_flags & MF_EXTERNAL_KEY));
-#else
-                    (options->cert_file != NULL) + (options->priv_key_file != NULL);
-#endif
+                    + ((options->priv_key_file != NULL) || (options->management_flags & MF_EXTERNAL_KEY));
 
                 if (sum == 0)
                 {
@@ -2983,14 +2971,14 @@  options_postprocess_verify_ce(const struct options *options,
             }
             else
             {
-#ifdef ENABLE_MANAGEMENT
                 if (!(options->management_flags & MF_EXTERNAL_CERT))
-#endif
-                notnull(options->cert_file, "certificate file (--cert) or PKCS#12 file (--pkcs12)");
-#ifdef ENABLE_MANAGEMENT
+                {
+                    notnull(options->cert_file, "certificate file (--cert) or PKCS#12 file (--pkcs12)");
+                }
                 if (!(options->management_flags & MF_EXTERNAL_KEY))
-#endif
-                notnull(options->priv_key_file, "private key file (--key) or PKCS#12 file (--pkcs12)");
+                {
+                    notnull(options->priv_key_file, "private key file (--key) or PKCS#12 file (--pkcs12)");
+                }
             }
         }
         if (ce->tls_auth_file && ce->tls_crypt_file)
@@ -3999,9 +3987,7 @@  options_postprocess_filechecks(struct options *options)
                                      options->extra_certs_file, R_OK,
                                      "--extra-certs");
 
-#ifdef ENABLE_MANAGMENT
     if (!(options->management_flags & MF_EXTERNAL_KEY))
-#endif
     {
         errs |= check_file_access_inline(options->priv_key_file_inline,
                                          CHKACC_FILE|CHKACC_PRIVATE,
@@ -5627,9 +5613,7 @@  bool
 key_is_external(const struct options *options)
 {
     bool ret = false;
-#ifdef ENABLE_MANAGEMENT
     ret = ret || (options->management_flags & MF_EXTERNAL_KEY);
-#endif
 #ifdef ENABLE_PKCS11
     ret = ret || (options->pkcs11_providers[0] != NULL);
 #endif
@@ -5836,7 +5820,6 @@  add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->management_flags |= MF_CONNECT_AS_CLIENT;
     }
-#ifdef ENABLE_MANAGEMENT
     else if (streq(p[0], "management-external-key"))
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
@@ -5885,7 +5868,6 @@  add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->management_flags |= MF_CLIENT_AUTH;
     }
-#endif /* ifdef ENABLE_MANAGEMENT */
     else if (streq(p[0], "management-log-cache") && p[1] && !p[2])
     {
         int cache;
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 68ad0cac..6f4b1f4a 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -438,10 +438,12 @@  struct options
     const char *management_client_user;
     const char *management_client_group;
 
-    /* Mask of MF_ values of manage.h */
-    unsigned int management_flags;
     const char *management_certificate;
 #endif
+    /* Mask of MF_ values of manage.h
+     * Always available to simplify options.c
+     */
+    unsigned int management_flags;
 
 #ifdef ENABLE_PLUGIN
     struct plugin_option_list *plugin_list;