Hi,

On Mon, Feb 6, 2023 at 6:24 AM Frank Lichtenheld <fr...@lichtenheld.com>
wrote:

> Automatically disabled when
> - iproute2 is enabled
>   (Don't want to force people specifying --disable-dco explicitely)
> - libnv is missing on FreeBSD
>   (FreeBSD version too old anyway)
>
> Will still error out if libnl-genl is missing on Linux to
> make people aware of new dependency.
>
> Signed-off-by: Frank Lichtenheld <fr...@lichtenheld.com>
> ---
>  configure.ac | 78 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 61 insertions(+), 17 deletions(-)
>
> v2: error out when libnl-genl is missing as discussed with ordex on
>     IRC.
>
>
> diff --git a/configure.ac b/configure.ac
> index 91500087..acfa4bc1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -144,14 +144,27 @@ AC_ARG_ENABLE(
>
>  AC_ARG_ENABLE(
>         [dco],
> -       [AS_HELP_STRING([--enable-dco], [enable data channel offload
> support using the ovpn-dco kernel module (always enabled on Windows)
> @<:@default=no@:>@])],
> +       [AS_HELP_STRING([--disable-dco], [enable data channel offload
> support using the ovpn-dco kernel module (always enabled on Windows)
> @<:@default=yes@:>@])],
>

The help string should be
"disable data channel offload support ..." instead of "enable data channel
offload ..." as it's now prefixed with "--disable-dco" .

The "default=yes" in this case may be better written as "default=auto" (or
"platform specific") but it's not critical. Here, default always refers to
the "--enable-FEATURE" form of the option. Confusing but consistent with
how other options are described.

We could be more helpful by describing both enable and disable forms of all
options, as well as custom args that the enable form takes (if any), but
that's beyond the scope of this patch.

-if test "$enable_dco" = "yes"; then
> +if test "$enable_dco" != "no"; then
> +       enable_dco_arg="$enable_dco"
> +       if test "${enable_iproute2}" = "yes"; then
> +               AC_MSG_WARN([iproute2 support cannot be enabled when using
> DCO])
>

This is mildly misleading as here iproute2 is going to win over DCO but the
message appears to indicate otherwise. It would result in outputs like:

WARNING: iproute2 support cannot be enabled when using DCO
WARNING: DCO support disabled
(or in an error)

Phrasing the first as "DCO cannot be enabled when using iproute2" would be
better.


> +               enable_dco="no"
> +       fi
> +       case "$host" in
> +               *-*-linux*)


I'll defer to those who understand DCO better to ACK the technical part.

Selva
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to