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

Acked-By: Arne Schwabe <>

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;
+    /* 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.


Openvpn-devel mailing list

Reply via email to