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.

Quoted 4 lines

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.

I wonder why we should bother to have a wrapper function here.

We can ship a lz4 library that has the new function, and if a system only provides an older version, declare it unsuitable (configure check) and use the bundled one.

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/[email protected]/msg14135.html

Signed-off-by David Sommerseth <[email protected]...>

---
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(-)

configure.ac +51 -21

index 6f1044e8..74443353 100644
@@ -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 <lz4.h>
+				 ]],
+				 [[
+/* 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

src/openvpn/comp-lz4.c +2 -1

index e056caa8..bdb3247d 100644
@@ -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

------------------------------------------------------------------------------
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 [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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.

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.

src/openvpn/comp-lz4.h +0 -0

 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

src/openvpn/options.c +3 -0

@@ -4121,11 +4121,17 @@
 #else
 #define LZO_LIB_VER_STR "", ""
 #endif
+#ifdef ENABLE_LZ4
+#define LZ4_LIB_VER_STR ", LZ4 ", LZ4_versionString()
+#else

Hi,

Quoted 4 lines · Simon Matter

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.

[..]

Quoted 10 lines

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.000000000 +0100
+++ openvpn-2.4.0/src/openvpn/comp-lz4.h 2017-01-17 09:11:12.000000000
+0100

src/openvpn/comp-lz4.h +0 -0

@@ -28,6 +28,10 @@
 
 #if defined(ENABLE_LZ4)
 

src/openvpn/comp-lz4.h line 35

Quoted 5 lines

src/openvpn/comp-lz4.h +4 -0

+#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.

Quoted 16 lines

src/openvpn/comp-lz4.h +0 -0

 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.000000000 +0100
+++ openvpn-2.4.0/src/openvpn/options.c 2017-01-17 09:11:12.000000000
+0100

src/openvpn/options.c +5 -0

@@ -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

src/openvpn/options.c line 4137

Quoted 10 lines

src/openvpn/options.c +2 -1

 
-    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
[email protected]...
fax: +49-89-35655025
[email protected]...

Quoted 3 lines

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.

Quoted 3 lines · Simon Matter

Quoted 2 lines

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.

Quoted 3 lines

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.

Hi,

returning to this patchset about printing the LZ4 library version...

Quoted 3 lines · Simon Matter

Quoted 3 lines · Simon Matter

Quoted 2 lines

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 still like this :-)

[..]

Quoted 10 lines

Quoted 1 line

Feature-ACK (I think this is a useful idea, for the same reasons we

have

Quoted 2 lines

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.

It now has... (1.7.5).

I'm not sure where LZ4_versionString() got introduced, though - their git repo does not have tags for 1.7.1 or 1.7.2, it is in the 1.7.3 tag, but not in the "last of the old numbering scheme" version.

Our configure.ac requires 1.7.1 - so if LZ4_versionString() is not in there yet, we might want to bump to 1.7.3 then.

Quoted 6 lines

It now has... (1.7.5).

I'm not sure where LZ4_versionString() got introduced, though - their git repo does not have tags for 1.7.1 or 1.7.2, it is in the 1.7.3 tag, but not in the "last of the old numbering scheme" version.

Quoted 2 lines

Our configure.ac requires 1.7.1 - so if LZ4_versionString() is not in there yet, we might want to bump to 1.7.3 then.

according to the NEWS file in lz4 there is no such thing as release 1.7.1 or 1.7.2, it goes from r131 -> v1.7.3. I therefore suggest to set the minimum version to 1.7.3 as I'm also unable to locate any official 1.7.1 or 1.7.2 release.

BTW, for 2.4.4 I've changed to patch to the one attached.

Regards,
Simon

src/openvpn/comp-lz4.c +1 -0

index e056caa8..bdb3247d 100644
@@ -43,6 +43,7 @@
 
 #include "memdbg.h"
 
+
 static void
 lz4_compress_init(struct compress_context *compctx)
 {

src/openvpn/comp-lz4.c +0 -0

         if (zlen <= 0)
         {
 

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/[email protected]/msg14135.html

Signed-off-by David Sommerseth <[email protected]...>

---
v2 - Don't use LZ4 version based #ifdef wrapper function
Do the LZ4 version check in ./configure