Re: [Openvpn-devel] [PATCH v2 3/3] Replace deprecated LZ4 function

2017-07-10 Thread Antonio Quartulli


On 10/07/17 15:07, Gert Doering wrote:
> Hi,
> 
> On Mon, Jul 10, 2017 at 02:33:43PM +0800, Antonio Quartulli wrote:
>> On 10/07/17 14:30, Gert Doering wrote:
>>> On Mon, Jul 10, 2017 at 12:31:31PM +0800, Antonio Quartulli wrote:
 Instead of cluttering the code with these ifdefs directly in the main
 codebase, how about doing it in compat.h, like this (it's a copy/paste -
 code might be wrapper by the mail client):
>>>
>>> Better, but I wonder if we shouldn't fix this in configure.ac - "if the
>>> system lz4 is older than what we bundle(*), use our compat-lz4.c instead"
>>>
>>> (*) ... or older than what we need, functionality-wise
>>
>> If we do this, then we jump into the can of worms that David S.
>> mentioned earlier, I think.
> 
> Uh, what can of worms specifically?  (David mentions lots of worms)
> 
> #ifdef in the code, or having funny wrapper functions hidden in .h files
> are worms as well - and these two are the ones that make the code hard
> to follow, so there's good reason to judge the trade-offs.
> 

I was referring to something he said in response to a comment to his v1:

if we use the bundled lz4, we might have a user stuck on our version
even though at some point he decided to update his system-wide lz4
library to a more recent release (maybe because of some security bugs).

The user at that point won't notice that openvpn is still running with
an older release.

> 
>> Generally speaking, why not just depending on "libX >= a.b.c." and error
>> out (at config time) if such lib is not available ?
>>
>> I think many other programs do exactly the same.
> 
> We are not "many other programs" :-) - and we decided, when we introduced
> lz4, that we want to bundle it as it's not a library installed commonly
> (yet).   And the whole point of "compat-*" stuff is "use it if the system is
> not adequate", without having to be turned-on explicitly.
> 
> We can change this, and remove the bundled compat-lz4* stuff, but that's a 
> different discussion.

Thanks for the explanation! I wasn't aware how we got to this
configuration :)


anyway, you are right. Deciding the faith of the bundled lib is probably
worth another thread/discussion.


Cheers,


-- 
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 3/3] Replace deprecated LZ4 function

2017-07-10 Thread Gert Doering
Hi,

On Mon, Jul 10, 2017 at 02:33:43PM +0800, Antonio Quartulli wrote:
> On 10/07/17 14:30, Gert Doering wrote:
> > On Mon, Jul 10, 2017 at 12:31:31PM +0800, Antonio Quartulli wrote:
> >> Instead of cluttering the code with these ifdefs directly in the main
> >> codebase, how about doing it in compat.h, like this (it's a copy/paste -
> >> code might be wrapper by the mail client):
> > 
> > Better, but I wonder if we shouldn't fix this in configure.ac - "if the
> > system lz4 is older than what we bundle(*), use our compat-lz4.c instead"
> > 
> > (*) ... or older than what we need, functionality-wise
> 
> If we do this, then we jump into the can of worms that David S.
> mentioned earlier, I think.

Uh, what can of worms specifically?  (David mentions lots of worms)

#ifdef in the code, or having funny wrapper functions hidden in .h files
are worms as well - and these two are the ones that make the code hard
to follow, so there's good reason to judge the trade-offs.


> Generally speaking, why not just depending on "libX >= a.b.c." and error
> out (at config time) if such lib is not available ?
> 
> I think many other programs do exactly the same.

We are not "many other programs" :-) - and we decided, when we introduced
lz4, that we want to bundle it as it's not a library installed commonly
(yet).   And the whole point of "compat-*" stuff is "use it if the system is
not adequate", without having to be turned-on explicitly.

We can change this, and remove the bundled compat-lz4* stuff, but that's a 
different discussion.

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 3/3] Replace deprecated LZ4 function

2017-07-10 Thread Antonio Quartulli
Hi,

On 10/07/17 14:30, Gert Doering wrote:
> Hi,
> 
> On Mon, Jul 10, 2017 at 12:31:31PM +0800, Antonio Quartulli wrote:
>> Instead of cluttering the code with these ifdefs directly in the main
>> codebase, how about doing it in compat.h, like this (it's a copy/paste -
>> code might be wrapper by the mail client):
> 
> Better, but I wonder if we shouldn't fix this in configure.ac - "if the
> system lz4 is older than what we bundle(*), use our compat-lz4.c instead"
> 
> (*) ... or older than what we need, functionality-wise

If we do this, then we jump into the can of worms that David S.
mentioned earlier, I think.

Generally speaking, why not just depending on "libX >= a.b.c." and error
out (at config time) if such lib is not available ?

I think many other programs do exactly the same.

If we still want, we could provide a configure switch that says "just
use the bundled lz4", this way people may use it without installing a
system-wide lib.


Cheers,

-- 
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 3/3] Replace deprecated LZ4 function

2017-07-10 Thread Gert Doering
Hi,

On Mon, Jul 10, 2017 at 12:31:31PM +0800, Antonio Quartulli wrote:
> Instead of cluttering the code with these ifdefs directly in the main
> codebase, how about doing it in compat.h, like this (it's a copy/paste -
> code might be wrapper by the mail client):

Better, but I wonder if we shouldn't fix this in configure.ac - "if the
system lz4 is older than what we bundle(*), use our compat-lz4.c instead"

(*) ... or older than what we need, functionality-wise

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 3/3] Replace deprecated LZ4 function

2017-07-09 Thread Antonio Quartulli
Hi,

On 22/02/17 03:27, David Sommerseth wrote:
> +#if defined LZ4_VERSION_NUMBER && LZ4_VERSION_NUMBER >= 10700
> +zlen = LZ4_compress_default((const char *)BPTR(buf), (char 
> *)BPTR(work), BLEN(buf), zlen_max );
> +#else
>  zlen = LZ4_compress_limitedOutput((const char *)BPTR(buf), (char 
> *)BPTR(work), BLEN(buf), zlen_max );
> +#endif

Instead of cluttering the code with these ifdefs directly in the main
codebase, how about doing it in compat.h, like this (it's a copy/paste -
code might be wrapper by the mail client):


diff --git a/src/compat/compat.h b/src/compat/compat.h
index d5228989..fa1b096e 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -70,4 +70,13 @@ int inet_pton(int af, const char *src, void *dst);

 #endif

+int ovpn_lz4_compress(const char *src, char *dst, int src_len, int
src_max_len)
+{
+#if defined LZ4_VERSION_NUMBER && LZ4_VERSION_NUMBER >= 10700
+return LZ4_compress_default(src, dst, src_len, src_max_len);
+#else
+return LZ4_compress_limitedOutput(src, dst, src_len, src_max_len);
+#endif
+}
+
 #endif /* COMPAT_H */
diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
index 6e40c325..a23a43c4 100644
--- a/src/openvpn/comp-lz4.c
+++ b/src/openvpn/comp-lz4.c
@@ -86,8 +86,8 @@ do_lz4_compress(struct buffer *buf,
 return false;
 }

-zlen = LZ4_compress_limitedOutput((const char *)BPTR(buf),
(char *)BPTR(work), BLEN(buf), zlen_max );
-
+zlen = ovpn_lz4_compress((const char *)BPTR(buf), (char
*)BPTR(work),
+ BLEN(buf), zlen_max );
 if (zlen <= 0)
 {
 dmsg(D_COMP_ERRORS, "LZ4 compression error");



This way we can put all similar changes in the same file and keep them
under control (IMHO we should avoid having #ifdefs directly in the
middle of the code as much as possible).

Cheers,


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


[Openvpn-devel] [PATCH v2 3/3] Replace deprecated LZ4 function

2017-02-21 Thread David Sommerseth
From: Christian Hesse 

The LZ4 function LZ4_compress_limitedOutput() is deprecated, compiler
gives warning:

warning: ‘LZ4_compress_limitedOutput’ is deprecated: use
LZ4_compress_default() instead

The new function LZ4_compress_default() appeared in r129 (1.7.0), so
replace the function there.

Signed-off-by: Christian Hesse 
---
 src/openvpn/comp-lz4.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
index e1f83e7..41b7d1b 100644
--- a/src/openvpn/comp-lz4.c
+++ b/src/openvpn/comp-lz4.c
@@ -87,7 +87,11 @@ do_lz4_compress(struct buffer *buf,
 return false;
 }
 
+#if defined LZ4_VERSION_NUMBER && LZ4_VERSION_NUMBER >= 10700
+zlen = LZ4_compress_default((const char *)BPTR(buf), (char 
*)BPTR(work), BLEN(buf), zlen_max );
+#else
 zlen = LZ4_compress_limitedOutput((const char *)BPTR(buf), (char 
*)BPTR(work), BLEN(buf), zlen_max );
+#endif
 
 if (zlen <= 0)
 {
-- 
2.11.0


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