[Openvpn-devel] [PATCH] dco: move availability check to the end of check_option_conflict() function

[Openvpn-devel] [PATCH] dco: move availability check to the end of check_option_conflict() function

[Antonio Quartulli]

To better arrange the order DCO option conflict messages are printed, we
decided to first perform all needed checks on provided options and, only
at the end, if no conflict was detected, to check if DCO is really
available on the system.

This way a user gets prompted with all warnings about their
configuration first and, when everything is fixed, they will see if DCO
is available or not.

While at it, compress the first check in just one if to make the code
simpler.

Signed-off-by: Antonio Quartulli <a@...2181...>
---
 src/openvpn/dco.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index a6912d4e..fdf474ca 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -268,18 +268,11 @@ dco_check_option_conflict_ce(const struct connection_entry *ce, int msglevel)
 bool
 dco_check_option_conflict(int msglevel, const struct options *o)
 {
-    if (o->tuntap_options.disable_dco)
-    {
-        /* already disabled by --disable-dco, no need to print warnings */
-        return false;
-    }
-
-    if (!dco_available(msglevel))
-    {
-        return false;
-    }
-
-    if (!o->dev)
+    /* check if DCO was already disabled by the user or if no dev name was
+     * specified at all. In the latter case, later logic will most likely stop
+     * OpenVPN, so no need to print any message here.
+     */
+    if (o->tuntap_options.disable_dco || !o->dev)
     {
         return false;
     }
@@ -361,7 +354,10 @@ dco_check_option_conflict(int msglevel, const struct options *o)
     }
     gc_free(&gc);
 
-    return true;
+    /* now that all options have been confirmed to be supported, check if DCO is
+     * truly available on the system
+     */
+    return dco_available(o);
 }
 
 bool
-- 
2.35.1

[Openvpn-devel] [PATCH v2] dco: move availability check to the end of check_option_conflict() function

[Antonio Quartulli]

To better arrange the order DCO option conflict messages are printed, we
decided to first perform all needed checks on provided options and, only
at the end, if no conflict was detected, to check if DCO is really
available on the system.

This way a user gets prompted with all warnings about their
configuration first and, when everything is fixed, they will see if DCO
is available or not.

While at it, compress the first check in just one if to make the code
simpler.

Signed-off-by: Antonio Quartulli <a@...2181...>
---

Changes from v1:
* pass proper argument to dco_available()
---
 src/openvpn/dco.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index a6912d4e..0877f0af 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -268,18 +268,11 @@ dco_check_option_conflict_ce(const struct connection_entry *ce, int msglevel)
 bool
 dco_check_option_conflict(int msglevel, const struct options *o)
 {
-    if (o->tuntap_options.disable_dco)
-    {
-        /* already disabled by --disable-dco, no need to print warnings */
-        return false;
-    }
-
-    if (!dco_available(msglevel))
-    {
-        return false;
-    }
-
-    if (!o->dev)
+    /* check if DCO was already disabled by the user or if no dev name was
+     * specified at all. In the latter case, later logic will most likely stop
+     * OpenVPN, so no need to print any message here.
+     */
+    if (o->tuntap_options.disable_dco || !o->dev)
     {
         return false;
     }
@@ -361,7 +354,10 @@ dco_check_option_conflict(int msglevel, const struct options *o)
     }
     gc_free(&gc);
 
-    return true;
+    /* now that all options have been confirmed to be supported, check if DCO is
+     * truly available on the system
+     */
+    return dco_available(msglevel);
 }
 
 bool
-- 
2.35.1

Re: [Openvpn-devel] [PATCH v2] dco: move availability check to the end of check_option_conflict() function

[Frank Lichtenheld]

FWIW I put this through the buildbot as a test for the new
extended t_client tests on the docker workers and it caused
no issues. Also ran the t_client tests on my DCO-enabled
Ubuntu 22 laptop. I did not do any more specific tests.

Changes look sensible to me, so
Acked-By: Frank Lichtenheld <frank@...2641...>

On Tue, Aug 02, 2022 at 03:03:12PM +0200, Antonio Quartulli wrote:
> To better arrange the order DCO option conflict messages are printed, we
> decided to first perform all needed checks on provided options and, only
> at the end, if no conflict was detected, to check if DCO is really
> available on the system.
> 
> This way a user gets prompted with all warnings about their
> configuration first and, when everything is fixed, they will see if DCO
> is available or not.
> 
> While at it, compress the first check in just one if to make the code
> simpler.
> 
> Signed-off-by: Antonio Quartulli <a@...2181...>
> ---
> 
> Changes from v1:
> * pass proper argument to dco_available()
> ---
>  src/openvpn/dco.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
> index a6912d4e..0877f0af 100644
> --- a/src/openvpn/dco.c
> +++ b/src/openvpn/dco.c
> @@ -268,18 +268,11 @@ dco_check_option_conflict_ce(const struct connection_entry *ce, int msglevel)
>  bool
>  dco_check_option_conflict(int msglevel, const struct options *o)
>  {
> -    if (o->tuntap_options.disable_dco)
> -    {
> -        /* already disabled by --disable-dco, no need to print warnings */
> -        return false;
> -    }
> -
> -    if (!dco_available(msglevel))
> -    {
> -        return false;
> -    }
> -
> -    if (!o->dev)
> +    /* check if DCO was already disabled by the user or if no dev name was
> +     * specified at all. In the latter case, later logic will most likely stop
> +     * OpenVPN, so no need to print any message here.
> +     */
> +    if (o->tuntap_options.disable_dco || !o->dev)
>      {
>          return false;
>      }
> @@ -361,7 +354,10 @@ dco_check_option_conflict(int msglevel, const struct options *o)
>      }
>      gc_free(&gc);
>  
> -    return true;
> +    /* now that all options have been confirmed to be supported, check if DCO is
> +     * truly available on the system
> +     */
> +    return dco_available(msglevel);
>  }
>  

-- 
  Frank Lichtenheld

[Openvpn-devel] [PATCH applied] Re: dco: move availability check to the end of check_option_conflict() function

[Gert Doering]

Thanks.  This was left hanging in the cold for some reason... merged
now.  I've subjected it to the usual test for DCO related stuff (client
with no-dco kernel, client with dco, server with dco) and verified that
the same instances have DCO enabled that had before - glad for Arne's
GLOBAL_STATS patch now :-)

This *should* be safe even on windows, as windows patch 1/7 changes
the first condition to "if (!dco_enabled(o) ...)" so it won't abort on
DCO-incompatible option in a config that does not have "windows-driver dco"
in the first place.

I have rewrapped the comment to better fit into 80 char line constraints
(since the second line was quite short)

Your patch has been applied to the master branch.

commit 78b8d0e162e1fa34780ee3d3ea84691539a0b1f3
Author: Antonio Quartulli
Date:   Tue Aug 2 15:03:12 2022 +0200

     dco: move availability check to the end of check_option_conflict() function

     Signed-off-by: Antonio Quartulli <a@...2181...>
     Acked-by: Frank Lichtenheld <frank@...2641...>
     Message-Id: <20220802130312.18871-1-a@...2181...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24783.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering