| Message ID | 20211207121137.3221-9-a@unstable.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce ovpn-dco(-win) support | expand |
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
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,
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])
Signed-off-by: Antonio Quartulli <a@unstable.cc> --- configure.ac | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-)