Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
Hi, On Sat, Sep 23, 2017 at 10:52:16AM +0200, Simon Matter wrote: > > Feature-ACK (I think this is a useful idea, for the same reasons we have > > LZO and OpenSSL versions :-) ), but that will not work if we use > > compat-lz4.h, so "code NAK". > > IIRC at the time of writing this, the embedded lz4 was too old and didn't > have the LZ4_versionString() function. So the code won't work with the > patch. If the embedded code has LZ4_versionString() now, it should be ok. Well, it will still lead to compile blowup if you try to include a non-existing "lz4.h" - no matter if the embedded version has LZ4_versionString() or not... But indeed, we should upgrade bundled LZ4. We have a script for that, we just never used it. For whatever reason. gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.de signature.asc Description: PGP signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
> Hi, > > On Thu, Sep 07, 2017 at 10:40:21PM +0200, Simon Matter wrote: >> While we are at it, I found it useful to see the used LZ4 version at >> runtime as it is done with LZO and other libraries. >> >> I've patched my rpms with the patch attached. > [..] > >> diff -Naur openvpn-2.4.0.orig/src/openvpn/comp-lz4.h >> openvpn-2.4.0/src/openvpn/comp-lz4.h >> --- openvpn-2.4.0.orig/src/openvpn/comp-lz4.h2016-12-26 >> 12:51:00.0 +0100 >> +++ openvpn-2.4.0/src/openvpn/comp-lz4.h 2017-01-17 09:11:12.0 >> +0100 >> @@ -28,6 +28,10 @@ >> >> #if defined(ENABLE_LZ4) >> >> +#if defined(HAVE_LZ4_H) >> +#include "lz4.h" >> +#endif >> + >> #include "buffer.h" > > Feature-ACK (I think this is a useful idea, for the same reasons we have > LZO and OpenSSL versions :-) ), but that will not work if we use > compat-lz4.h, so "code NAK". IIRC at the time of writing this, the embedded lz4 was too old and didn't have the LZ4_versionString() function. So the code won't work with the patch. If the embedded code has LZ4_versionString() now, it should be ok. Regards, Simon > > comp-lz4.c currently has > > #if defined(NEED_COMPAT_LZ4) > #include "compat-lz4.h" > #else > #include "lz4.h" > #endif > > which might just be moved to comp-lz4.h, and then things should work. > > >> extern const struct compress_alg lz4_alg; >> diff -Naur openvpn-2.4.0.orig/src/openvpn/options.c >> openvpn-2.4.0/src/openvpn/options.c >> --- openvpn-2.4.0.orig/src/openvpn/options.c 2016-12-26 >> 12:51:00.0 +0100 >> +++ openvpn-2.4.0/src/openvpn/options.c 2017-01-17 09:11:12.0 >> +0100 >> @@ -4121,11 +4121,17 @@ >> #else >> #define LZO_LIB_VER_STR "", "" >> #endif >> +#ifdef ENABLE_LZ4 >> +#define LZ4_LIB_VER_STR ", LZ4 ", LZ4_versionString() >> +#else >> +#define LZ4_LIB_VER_STR "", "" >> +#endif >> >> -msg(flags, "library versions: %s%s%s", SSL_LIB_VER_STR, >> LZO_LIB_VER_STR); >> +msg(flags, "library versions: %s%s%s%s%s", SSL_LIB_VER_STR, >> LZO_LIB_VER_STR, LZ4_LIB_VER_STR); >> >> #undef SSL_LIB_VER_STR >> #undef LZO_LIB_VER_STR >> +#undef LZ4_LIB_VER_STR >> } > > That part looks good. > > gert > > > -- > USENET is *not* the non-clickable part of WWW! >//www.muc.de/~gert/ > Gert Doering - Munich, Germany > g...@greenie.muc.de > fax: +49-89-35655025 > g...@net.informatik.tu-muenchen.de > -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
Hi,
On Thu, Sep 07, 2017 at 07:20:04PM +0200, David Sommerseth wrote:
> We are using a deprecated function, LZ4_compress_limitedOutput(), which
> will be removed with time. The correct function to use is
> LZ4_compress_default().
> Both function takes the same number of arguments and data types, so the change
> is minimal.
>
> This patch will also enforce the system LZ4 library to be at least v1.7.1. If
> the system library is not found or it is older, it will be build using the
> bundled
> LZ4 library. The version number requirement is based on the LZ4 version we
> ship.
>
> The changes in configure.ac for the version check is modelled around the same
> approach we use for OpenSSL. Plus it does a few minor reformats and
> improvements
> to comply with more recommend autoconf coding style.
>
> This patch is a result of the discussions in this mail thread:
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14135.html
>
> Signed-off-by: David Sommerseth
>
> ---
> v2 - Don't use LZ4 version based #ifdef wrapper function
> Do the LZ4 version check in ./configure
This looks good to me.
I have tested on
- FreeBSD with lz4 installed, lz4.pc -> uses system lib
- FreeBSD with lz4 installed, no lz4.pc, lz4 stuff in LZ4_CFLAGS/LIBS
-> uses system lib
- FreeBSD with lz4 installed, but not in include path
-> uses compat-lz4.*
- Gentoo Linux with lz4 installed, lz4.pc
-> uses system lib
- Gentoo Linux with lz4 installed, no lz4.pc, but lz4 in /usr/{include,lib}/
-> uses system lib
- Gentoo Linux with no lz4 installed -> uses compat-lz4.*
In all cases, the system builds and produces a working openvpn binary.
The messages in case "2" and "5" are slightly weird, though...
(2)
checking lz4.h usability... yes
checking lz4.h presence... no
configure: WARNING: lz4.h: accepted by the compiler, rejected by the
preprocessor!
configure: WARNING: lz4.h: proceeding with the compiler's result
checking for lz4.h... yes
checking additionally if system LZ4 version >= 1.7.1... ok
(5)
checking for LZ4... no
checking lz4.h usability... yes
checking lz4.h presence... yes
checking for lz4.h... yes
checking additionally if system LZ4 version >= 1.7.1... ok
checking for LZ4_compress in -llz4... yes
(but this is configure...)
There's an extra added blank line in comp-lz4.c and the bracket whitespace
Antonio complained about - but those can be fixed on commit.
So, ACK. Thanks for wading through this.
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
Hi, On Thu, Sep 07, 2017 at 10:40:21PM +0200, Simon Matter wrote: > While we are at it, I found it useful to see the used LZ4 version at > runtime as it is done with LZO and other libraries. > > I've patched my rpms with the patch attached. [..] > diff -Naur openvpn-2.4.0.orig/src/openvpn/comp-lz4.h > openvpn-2.4.0/src/openvpn/comp-lz4.h > --- openvpn-2.4.0.orig/src/openvpn/comp-lz4.h 2016-12-26 12:51:00.0 > +0100 > +++ openvpn-2.4.0/src/openvpn/comp-lz4.h 2017-01-17 09:11:12.0 > +0100 > @@ -28,6 +28,10 @@ > > #if defined(ENABLE_LZ4) > > +#if defined(HAVE_LZ4_H) > +#include "lz4.h" > +#endif > + > #include "buffer.h" Feature-ACK (I think this is a useful idea, for the same reasons we have LZO and OpenSSL versions :-) ), but that will not work if we use compat-lz4.h, so "code NAK". comp-lz4.c currently has #if defined(NEED_COMPAT_LZ4) #include "compat-lz4.h" #else #include "lz4.h" #endif which might just be moved to comp-lz4.h, and then things should work. > extern const struct compress_alg lz4_alg; > diff -Naur openvpn-2.4.0.orig/src/openvpn/options.c > openvpn-2.4.0/src/openvpn/options.c > --- openvpn-2.4.0.orig/src/openvpn/options.c 2016-12-26 12:51:00.0 > +0100 > +++ openvpn-2.4.0/src/openvpn/options.c 2017-01-17 09:11:12.0 > +0100 > @@ -4121,11 +4121,17 @@ > #else > #define LZO_LIB_VER_STR "", "" > #endif > +#ifdef ENABLE_LZ4 > +#define LZ4_LIB_VER_STR ", LZ4 ", LZ4_versionString() > +#else > +#define LZ4_LIB_VER_STR "", "" > +#endif > > -msg(flags, "library versions: %s%s%s", SSL_LIB_VER_STR, LZO_LIB_VER_STR); > +msg(flags, "library versions: %s%s%s%s%s", SSL_LIB_VER_STR, > LZO_LIB_VER_STR, LZ4_LIB_VER_STR); > > #undef SSL_LIB_VER_STR > #undef LZO_LIB_VER_STR > +#undef LZ4_LIB_VER_STR > } That part looks good. gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.de signature.asc Description: PGP signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
On 07/09/17 22:40, Simon Matter wrote: > Hi, > > While we are at it, I found it useful to see the used LZ4 version at > runtime as it is done with LZO and other libraries. > > I've patched my rpms with the patch attached. Thanks a lot! I think this makes sense. But I think we can do this stuff as a separate patch, not part of this round of patches. Lets target this after we have sorted out the current LZ4 patches have been applied. Then it is much easier to test and validate this approach. -- kind regards, David Sommerseth OpenVPN Technologies, Inc signature.asc Description: OpenPGP digital signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
Hi,
On 08/09/17 01:20, David Sommerseth wrote:
[CUT]
> diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
> index e056caa8..bdb3247d 100644
> --- a/src/openvpn/comp-lz4.c
> +++ b/src/openvpn/comp-lz4.c
> @@ -43,6 +43,7 @@
>
> #include "memdbg.h"
>
> +
> static void
> lz4_compress_init(struct compress_context *compctx)
> {
> @@ -86,7 +87,7 @@ do_lz4_compress(struct buffer *buf,
> return false;
> }
>
> -zlen = LZ4_compress_limitedOutput((const char *)BPTR(buf), (char
> *)BPTR(work), BLEN(buf), zlen_max );
> +zlen = LZ4_compress_default((const char *)BPTR(buf), (char
> *)BPTR(work), BLEN(buf), zlen_max );
while at it, could you remove the annoying space between the last
argument and the closing parenthesis? (you or the committer)
Cheers,
>
> if (zlen <= 0)
> {
>
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
Hi,
While we are at it, I found it useful to see the used LZ4 version at
runtime as it is done with LZO and other libraries.
I've patched my rpms with the patch attached.
Regards,
Simon
> We are using a deprecated function, LZ4_compress_limitedOutput(), which
> will be removed with time. The correct function to use is
> LZ4_compress_default().
> Both function takes the same number of arguments and data types, so the
> change
> is minimal.
>
> This patch will also enforce the system LZ4 library to be at least v1.7.1.
> If
> the system library is not found or it is older, it will be build using the
> bundled
> LZ4 library. The version number requirement is based on the LZ4 version
> we ship.
>
> The changes in configure.ac for the version check is modelled around the
> same
> approach we use for OpenSSL. Plus it does a few minor reformats and
> improvements
> to comply with more recommend autoconf coding style.
>
> This patch is a result of the discussions in this mail thread:
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14135.html
>
> Signed-off-by: David Sommerseth
>
> ---
> v2 - Don't use LZ4 version based #ifdef wrapper function
> Do the LZ4 version check in ./configure
> ---
> configure.ac | 72
> +++---
> src/openvpn/comp-lz4.c | 3 ++-
> 2 files changed, 53 insertions(+), 22 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 6f1044e8..74443353 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1088,37 +1088,67 @@ dnl
> AC_ARG_VAR([LZ4_CFLAGS], [C compiler flags for lz4])
> AC_ARG_VAR([LZ4_LIBS], [linker flags for lz4])
> if test "$enable_lz4" = "yes" && test "$enable_comp_stub" = "no"; then
> -AC_CHECKING([for LZ4 Library and Header files])
> -havelz4lib=1
> -
> -# if LZ4_LIBS is set, we assume it will work, otherwise test
> -if test -z "${LZ4_LIBS}"; then
> - AC_CHECK_LIB(lz4, LZ4_compress,
> - [ LZ4_LIBS="-llz4" ],
> - [
> - AC_MSG_RESULT([LZ4 library not found.])
> - havelz4lib=0
> - ])
> +if test -z "${LZ4_CFLAGS}" -a -z "${LZ4_LIBS}"; then
> + # if the user did not explicitly specify flags, try to autodetect
> + PKG_CHECK_MODULES([LZ4],
> + [liblz4 >= 1.7.1],
> + [have_lz4="yes"],
> + [] # If this fails, we will do another test next
> + )
> fi
>
> saved_CFLAGS="${CFLAGS}"
> +saved_LIBS="${LIBS}"
> CFLAGS="${CFLAGS} ${LZ4_CFLAGS}"
> -AC_CHECK_HEADERS(lz4.h,
> - ,
> - [
> -AC_MSG_RESULT([LZ4 headers not found.])
> -havelz4lib=0
> - ])
> -
> -if test $havelz4lib = 0 ; then
> - AC_MSG_RESULT([LZ4 library or header not found, using version in
> src/compat/compat-lz4.*])
> +LIBS="${LIBS} ${LZ4_LIBS}"
> +
> +# If pkgconfig check failed or LZ4_CFLAGS/LZ4_LIBS env vars
> +# are used, check the version directly in the LZ4 include file
> +if test "${have_lz4}" != "yes"; then
> + AC_CHECK_HEADERS([lz4.h],
> + [have_lz4h="yes"],
> + [])
> +
> + if test "${have_lz4h}" = "yes" ; then
> + AC_MSG_CHECKING([additionally if system LZ4 version >= 1.7.1])
> + AC_COMPILE_IFELSE(
> + [AC_LANG_PROGRAM([[
> +#include
> + ]],
> + [[
> +/* Version encoding: MMNNPP (Major miNor Patch) - see lz4.h for details
> */
> +#if LZ4_VERSION_NUMBER < 10701L
> +#error LZ4 is too old
> +#endif
> + ]]
> + )],
> + [
> + AC_MSG_RESULT([ok])
> + have_lz4="yes"
> + ],
> + [AC_MSG_RESULT([system LZ4 library is too old])]
> + )
> + fi
> +fi
> +
> +# if LZ4_LIBS is set, we assume it will work, otherwise test
> +if test -z "${LZ4_LIBS}"; then
> + AC_CHECK_LIB([lz4],
> + [LZ4_compress],
> + [LZ4_LIBS="-llz4"],
> + [have_lz4="no"])
> +fi
> +
> +if test "${have_lz4}" != "yes" ; then
> + AC_MSG_RESULT([ usuable LZ4 library or header not found, using
> version
> in src/compat/compat-lz4.*])
> AC_DEFINE([NEED_COMPAT_LZ4], [1], [use copy of LZ4 source in compat/])
> LZ4_LIBS=""
> fi
> OPTIONAL_LZ4_CFLAGS="${LZ4_CFLAGS}"
> OPTIONAL_LZ4_LIBS="${LZ4_LIBS}"
> -AC_DEFINE(ENABLE_LZ4, 1, [Enable LZ4 compression library])
> +AC_DEFINE(ENABLE_LZ4, [1], [Enable LZ4 compression library])
> CFLAGS="${saved_CFLAGS}"
> +LIBS="${saved_LIBS}"
> fi
>
>
> diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
> index e056caa8..bdb3247d 100644
> --- a/src/openvpn/comp-lz4.c
> +++ b/src/openvpn/comp-lz4.c
> @@ -43,6 +43,7 @@
>
> #include "memdbg.h"
>
> +
> static void
> lz4_compress_init(struct compress_context *compctx)
[Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API
We are using a deprecated function, LZ4_compress_limitedOutput(), which
will be removed with time. The correct function to use is
LZ4_compress_default().
Both function takes the same number of arguments and data types, so the change
is minimal.
This patch will also enforce the system LZ4 library to be at least v1.7.1. If
the system library is not found or it is older, it will be build using the
bundled
LZ4 library. The version number requirement is based on the LZ4 version we
ship.
The changes in configure.ac for the version check is modelled around the same
approach we use for OpenSSL. Plus it does a few minor reformats and
improvements
to comply with more recommend autoconf coding style.
This patch is a result of the discussions in this mail thread:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14135.html
Signed-off-by: David Sommerseth
---
v2 - Don't use LZ4 version based #ifdef wrapper function
Do the LZ4 version check in ./configure
---
configure.ac | 72 +++---
src/openvpn/comp-lz4.c | 3 ++-
2 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/configure.ac b/configure.ac
index 6f1044e8..74443353 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1088,37 +1088,67 @@ dnl
AC_ARG_VAR([LZ4_CFLAGS], [C compiler flags for lz4])
AC_ARG_VAR([LZ4_LIBS], [linker flags for lz4])
if test "$enable_lz4" = "yes" && test "$enable_comp_stub" = "no"; then
-AC_CHECKING([for LZ4 Library and Header files])
-havelz4lib=1
-
-# if LZ4_LIBS is set, we assume it will work, otherwise test
-if test -z "${LZ4_LIBS}"; then
- AC_CHECK_LIB(lz4, LZ4_compress,
- [ LZ4_LIBS="-llz4" ],
- [
- AC_MSG_RESULT([LZ4 library not found.])
- havelz4lib=0
- ])
+if test -z "${LZ4_CFLAGS}" -a -z "${LZ4_LIBS}"; then
+ # if the user did not explicitly specify flags, try to autodetect
+ PKG_CHECK_MODULES([LZ4],
+ [liblz4 >= 1.7.1],
+ [have_lz4="yes"],
+ [] # If this fails, we will do another test next
+ )
fi
saved_CFLAGS="${CFLAGS}"
+saved_LIBS="${LIBS}"
CFLAGS="${CFLAGS} ${LZ4_CFLAGS}"
-AC_CHECK_HEADERS(lz4.h,
- ,
- [
- AC_MSG_RESULT([LZ4 headers not found.])
- havelz4lib=0
- ])
-
-if test $havelz4lib = 0 ; then
- AC_MSG_RESULT([LZ4 library or header not found, using version in
src/compat/compat-lz4.*])
+LIBS="${LIBS} ${LZ4_LIBS}"
+
+# If pkgconfig check failed or LZ4_CFLAGS/LZ4_LIBS env vars
+# are used, check the version directly in the LZ4 include file
+if test "${have_lz4}" != "yes"; then
+ AC_CHECK_HEADERS([lz4.h],
+[have_lz4h="yes"],
+[])
+
+ if test "${have_lz4h}" = "yes" ; then
+ AC_MSG_CHECKING([additionally if system LZ4 version >= 1.7.1])
+ AC_COMPILE_IFELSE(
+ [AC_LANG_PROGRAM([[
+#include
+]],
+[[
+/* Version encoding: MMNNPP (Major miNor Patch) - see lz4.h for details */
+#if LZ4_VERSION_NUMBER < 10701L
+#error LZ4 is too old
+#endif
+]]
+ )],
+ [
+ AC_MSG_RESULT([ok])
+ have_lz4="yes"
+ ],
+ [AC_MSG_RESULT([system LZ4 library is too old])]
+ )
+ fi
+fi
+
+# if LZ4_LIBS is set, we assume it will work, otherwise test
+if test -z "${LZ4_LIBS}"; then
+ AC_CHECK_LIB([lz4],
+[LZ4_compress],
+[LZ4_LIBS="-llz4"],
+[have_lz4="no"])
+fi
+
+if test "${have_lz4}" != "yes" ; then
+ AC_MSG_RESULT([ usuable LZ4 library or header not found, using
version in src/compat/compat-lz4.*])
AC_DEFINE([NEED_COMPAT_LZ4], [1], [use copy of LZ4 source in compat/])
LZ4_LIBS=""
fi
OPTIONAL_LZ4_CFLAGS="${LZ4_CFLAGS}"
OPTIONAL_LZ4_LIBS="${LZ4_LIBS}"
-AC_DEFINE(ENABLE_LZ4, 1, [Enable LZ4 compression library])
+AC_DEFINE(ENABLE_LZ4, [1], [Enable LZ4 compression library])
CFLAGS="${saved_CFLAGS}"
+LIBS="${saved_LIBS}"
fi
diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
index e056caa8..bdb3247d 100644
--- a/src/openvpn/comp-lz4.c
+++ b/src/openvpn/comp-lz4.c
@@ -43,6 +43,7 @@
#include "memdbg.h"
+
static void
lz4_compress_init(struct compress_context *compctx)
{
@@ -86,7 +87,7 @@ do_lz4_compress(struct buffer *buf,
return false;
}
-zlen = LZ4_compress_limitedOutput((const char *)BPTR(buf), (char
*)BPTR(work), BLEN(buf), zlen_max );
+zlen = LZ4_compress_default((const char *)BPTR(buf), (char
*)BPTR(work), BLEN(buf), zlen_max );
if (zlen <= 0)
{
--
2.13.5
