[Openvpn-devel,RFC,8/8] ovpn-dco: force user to set DCO_INCLUDEDIR

Message ID 20211207121137.3221-9-a@unstable.cc
State Changes Requested
Headers show
Series
  • Introduce ovpn-dco(-win) support
Related show

Commit Message

Antonio Quartulli Dec. 7, 2021, 12:11 p.m.
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 configure.ac | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

Comments

Gert Doering Dec. 29, 2021, 1:34 p.m. | #1
Hi,

On Tue, Dec 07, 2021 at 01:11:37PM +0100, Antonio Quartulli wrote:
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>  configure.ac | 32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)

I think this should be squashed into the first patch, that adds DCO
support to configure.ac in the first place.

Plus, I wonder how we should do this in the first place - if we want
widespread DCO adoption, maybe we should include the necessary header
file into the openvpn community tree, and always compile "with DCO"
on Linux (if libnl is available)?

Having to fiddle with an extra library dependency *and* "you need this
header file which is found elsewhere" feels extra clumsy...

Alternatively, maybe linuxdco should just install the header file when
installing the kernel module?  How's this typically done for other
kernel modules that expose an API that needs a header file?

gert
Antonio Quartulli Dec. 29, 2021, 1:49 p.m. | #2
Hi,

On 29/12/2021 14:34, Gert Doering wrote:
> Hi,
> 
> On Tue, Dec 07, 2021 at 01:11:37PM +0100, Antonio Quartulli wrote:
>> Signed-off-by: Antonio Quartulli <a@unstable.cc>
>> ---
>>   configure.ac | 32 ++++++++++----------------------
>>   1 file changed, 10 insertions(+), 22 deletions(-)
> 
> I think this should be squashed into the first patch, that adds DCO
> support to configure.ac in the first place.
> 

Yeah, this is done already.

> Plus, I wonder how we should do this in the first place - if we want
> widespread DCO adoption, maybe we should include the necessary header
> file into the openvpn community tree, and always compile "with DCO"
> on Linux (if libnl is available)?
> 
> Having to fiddle with an extra library dependency *and* "you need this
> header file which is found elsewhere" feels extra clumsy...
> 
> Alternatively, maybe linuxdco should just install the header file when
> installing the kernel module?  How's this typically done for other
> kernel modules that expose an API that needs a header file?
> 

Normally the required header is just shipped with the program itself 
(i.e. check the 'iw' program which comes with its own copy of 'nl80211.h').

Being this API forward compatible, it won't hurt if our header is 
slightly out-of-date.

Ideally, when DCO will be upstream, the header will be installed 
automatically along with other kernel headers. But we still can't assume 
they are installed, hence it's just easier to ship the header with the 
OpenVPN source code.

I'd just do that at the end.

Regards,

Patch

diff --git a/configure.ac b/configure.ac
index b6ecb23a..1911f24e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -777,29 +777,22 @@  PKG_CHECK_MODULES(
 
 if test "$enable_dco" = "yes"; then
 dnl
-dnl  Configure path for the ovpn-dco kernel module source directory.
+dnl  Configure path for the ovpn-dco header directory.
 dnl
-dnl  This is similar to the core librariy, there is an embedded
-dnl  version in this tree which will be used by default.  The
-dnl  git checkout inside the ovpn-dco/ directory is managed via git
-dnl  submodule.
-dnl
-	AC_ARG_VAR([DCO_SOURCEDIR], [Alternative ovpn-dco kernel module source directory])
-	if test -z "${DCO_SOURCEDIR}"; then
-		case "$host" in
-			*-mingw*) DCO_SOURCEDIR="${srcdir}/../ovpn-dco-win";;
-			*) DCO_SOURCEDIR="${srcdir}/../ovpn-dco";;
-		esac
+	AC_ARG_VAR([DCO_INCLUDEDIR], [ovpn-dco header directory (where to find ovpn-dco.h)])
+	if test -z "${DCO_INCLUDEDIR}"; then
+		AC_MSG_ERROR([You must specify DCO_INCLUDEDIR when using --enable-dco.])
+
 	fi
-	AC_MSG_NOTICE([Using ovpn-dco source directory: ${DCO_SOURCEDIR}])
-	AC_SUBST([DCO_SOURCEDIR])
+
+	AC_MSG_NOTICE([Using ovpn-dco header directory: ${DCO_INCLUDEDIR}])
+	DCO_CFLAGS="-I${DCO_INCLUDEDIR}"
 
 	case "$host" in
 		*-*-linux*)
 dnl
 dnl Include generic netlink library used to talk to ovpn-dco
 dnl
-			saved_CFLAGS="${CFLAGS}"
 			PKG_CHECK_MODULES(
 				[LIBNL_GENL],
 				[libnl-genl-3.0 >= 3.2.29],
@@ -807,16 +800,12 @@  dnl
 				[AC_MSG_ERROR([libnl-genl-3.0 package not found or too old. Is the development package and pkg-config installed? Must be version 3.4.0 or newer])]
 			)
 
-			DCO_CFLAGS="-I${DCO_SOURCEDIR}/include/uapi ${LIBNL_GENL_CFLAGS}"
-
-			CFLAGS="${CFLAGS} ${DCO_CFLAGS}"
+			CFLAGS="${CFLAGS} ${DCO_CFLAGS} ${LIBNL_GENL_CFLAGS}"
 			AC_CHECK_HEADERS(
 				[linux/ovpn_dco.h],
 				,
-				[AC_MSG_ERROR([linux/ovpn_dco.h is missing (use DCO_SOURCE to set path to it, CFLAGS=${CFLAGS})])]
+				[AC_MSG_ERROR([linux/ovpn_dco.h couldn't be found (use DCO_INCLUDEDIR to set the path to it, CFLAGS=${CFLAGS})])]
 			)
-			CFLAGS=${saved_CFLAGS}
-
 			OPTIONAL_DCO_LIBS="${LIBNL_GENL_LIBS}"
 
 			AC_DEFINE(ENABLE_LINUXDCO, 1, [Enable linux data channel offload])
@@ -1407,7 +1396,6 @@  AC_SUBST([OPTIONAL_PKCS11_HELPER_CFLAGS])
 AC_SUBST([OPTIONAL_PKCS11_HELPER_LIBS])
 AC_SUBST([OPTIONAL_INOTIFY_CFLAGS])
 AC_SUBST([OPTIONAL_INOTIFY_LIBS])
-AC_SUBST([OPTIONAL_DCO_CFLAGS])
 AC_SUBST([OPTIONAL_DCO_LIBS])
 
 AC_SUBST([PLUGIN_AUTH_PAM_CFLAGS])