zeevt / csnappy Goto Github PK

We use a fork of your code embedded in the Sereal project.

We did some big endian changes. I thought I would at least point them out to you even if I do not have a patch for your code.

Sereal/Sereal@29c6929

The Sereal project is seeing consistent errors with csnappy when built with GCC 10. When compiling with gcc 9 or earlier all tests pass, when building with GCC 10 multiple users are reporting tests showing corrupted data.

At this time I do not have a gcc 10 to replicate, I am working on it, but I thought I should file a report early.

Note the code compiles fine on gcc10 it just fails tests. Which is a bit worrisome, and suggests a compiler bug to me.

Yves

Not sure what to do about Mingw. Perhaps add back ability to allow user to define endianness and fall back to generic implemention of byteswaps?

Reference error: http://www.cpantesters.org/cpan/report/8e2c57c8-6ce4-1014-b0a1-44f20063837e

As at the moment csnappy is used in few distributions IMO it woild be good to make first release to better track changes in those packages.

The public header file csnappy.h uses unit32_t type. The type is declared in standard <stdint.h> header file which needs to be included to get the type available. Otherwise compilers will complain:

/usr/include/csnappy.h:24:1: error: unknown type name ‘uint32_t’

Please consider this one-line patch:

From 911a1eb1b120ac956369ca77be8a4500c3bd1f1d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <[email protected]>
Date: Mon, 13 Oct 2014 16:17:29 +0200
Subject: [PATCH] Include <stdint.h> for uint32_t types
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Petr Písař <[email protected]>

---
 csnappy.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/csnappy.h b/csnappy.h
index 8920b37..28ad3e1 100644
--- a/csnappy.h
+++ b/csnappy.h
@@ -17,6 +17,8 @@ extern "C" {
 #define __attribute__(x) /*NOTHING*/
 #endif

+#include <stdint.h>
+
 /*
  * Returns the maximal size of the compressed representation of
  * input data that is "source_len" bytes in length;
-- 
1.9.3

$ diff -u csnappy.h csnappy.h.new
--- csnappy.h Tue Mar 13 11:20:46 2012
+++ csnappy.h.new Tue Mar 13 11:20:37 2012
@@ -13,6 +13,11 @@
#define CSNAPPY_WORKMEM_BYTES_POWER_OF_TWO 15
#define CSNAPPY_WORKMEM_BYTES (1 << CSNAPPY_WORKMEM_BYTES_POWER_OF_TWO)

+#ifndef GNUC
+#define attribute(x) /NOTHING/
+#define __builtin_expect(a,b) a
+#endif
+
/*

• Returns the maximal size of the compressed representation of
• input data that is "source_len" bytes in length;
  $

diff -ur Compress-Snappy-0.12/src/csnappy_internal_userspace.h
Compress-Snappy-0.12-bnjf/src/csnappy_internal_userspace.h
--- Compress-Snappy-0.12/src/csnappy_internal_userspace.h
2011-04-30 05:21:44.000000000 +1000
+++ Compress-Snappy-0.12-bnjf/src/csnappy_internal_userspace.h
2011-12-01 10:28:56.092213000 +1100
@@ -162,6 +162,11 @@
#define bswap_16(x) OSSwapInt16(x)
#define bswap_32(x) OSSwapInt32(x)
#define bswap_64(x) OSSwapInt64(x)
+#elif defined(__sun)
+#include <sys/byteorder.h>
+#define bswap_16(x) BSWAP_16(x)
+#define bswap_32(x) BSWAP_32(x)
+#define bswap_64(x) BSWAP_64(x)
#else
#include <byteswap.h>
#endif

csnappy is broken on 64 bit big endian machines, Z/Os, Sparc, etc.

See Pull Request #17 for a fix.

What is the maximum size for the compressed and/or uncompressed that this library is supporting? I think it would be nice if we document the desired limits and make the implementation match the documentation.

The snappy format description specifies that the maximum size of the uncompressed data is 2^32 - 1:

https://code.google.com/p/snappy/source/browse/trunk/format_description.txt

The compressed data can be > 2^32(if the uncompressed data is random enough).

In my tests csnappy seems to be able to compress data of 3G, but it is not able to decompress it back. I think the limitation is here:

https://github.com/zeevt/csnappy/blob/master/csnappy_decompress.c#L329

'available' is declared an an signed int and if it is > 2Gb it becomes negative. Is this the desired behaviour(csnappy supports input of maximum 2GB) or a bug?
If you need more details please let me know.

Hello,

the Makfile installs the library and the header file into a locations which are not suitable for everyone. Following patch adds DESTIDIR and LIBDIR variables which allows to redefine the locations. This is handy for Linux distributions.

From a2e679af00bf0ed2d98c8ada4f6c9daf8d5ff3e7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <[email protected]>
Date: Mon, 13 Oct 2014 11:07:20 +0200
Subject: [PATCH] Install file into DESTDIR and LIBDIR
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This patch adds two new "make install" arguments:

LIBDIR to specify path for library (/usr/lib by default)
DESTDIR to prepend a path for all installed files as used by packagers
(default is none).

E.g. "make install DESTDIR=/tmp/dest LIBDIR=/usr/lib64" will install:

/tmp/dest/usr/include/csnappy.h
/tmp/dest/usr/lib64/libcsnappy.so

So one can collect /tmp/dest/ content and distribute it as tar
archives or any other pre-compiled package.

Signed-off-by: Petr Písař <[email protected]>

---
 Makefile | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index f93c3cc..15600d5 100644
--- a/Makefile
+++ b/Makefile
@@ -8,6 +8,8 @@ else
 CFLAGS += $(OPT_FLAGS)
 endif
 LDFLAGS = -Wl,-O1 -Wl,--no-undefined
+PREFIX := /usr
+LIBDIR := $(PREFIX)/lib

 all: test

@@ -88,12 +90,14 @@ unaligned_test_android: unaligned_test.c unaligned_arm.s
    -Wl,--no-undefined -Wl,-z,noexecstack -lc -lm -o unaligned_test_android

 install: csnappy.h libcsnappy.so
-   cp csnappy.h /usr/include/
-   cp libcsnappy.so /usr/lib/
+   install -d "$(DESTDIR)$(PREFIX)"/include
+   install -m 0644 csnappy.h "$(DESTDIR)$(PREFIX)"/include/
+   install -d "$(DESTDIR)$(LIBDIR)"
+   install libcsnappy.so "$(DESTDIR)$(LIBDIR)"

 uninstall:
-   rm -f /usr/include/csnappy.h
-   rm -f /usr/lib/libcsnappy.so
+   rm -f "$(DESTDIR)$(PREFIX)"/include/csnappy.h
+   rm -f "$(DESTDIR)$(LIBDIR)"/libcsnappy.so

 clean:
    rm -f *.o *_debug libcsnappy.so cl_tester
-- 
1.9.3

Building latest master HEAD (354b95d) fails on 64-bit ARM:

$ make
cc -std=gnu89 -Wall -pedantic -DHAVE_BUILTIN_CTZ -g -O2 -DNDEBUG -fomit-frame-pointer -fPIC -DPIC -c -o csnappy_compress.o csnappy_compress.c
cc -std=gnu89 -Wall -pedantic -DHAVE_BUILTIN_CTZ -g -O2 -DNDEBUG -fomit-frame-pointer -fPIC -DPIC -c -o csnappy_decompress.o csnappy_decompress.c
cc -std=gnu89 -Wall -pedantic -DHAVE_BUILTIN_CTZ -g -O2 -DNDEBUG -fomit-frame-pointer -Wl,-O1 -Wl,--no-undefined -shared -o libcsnappy.so csnappy_compress.o csnappy_decompress.o
cc -std=gnu89 -Wall -pedantic -DHAVE_BUILTIN_CTZ -g -O2 -DNDEBUG -fomit-frame-pointer -Wl,-O1 -Wl,--no-undefined -D_GNU_SOURCE -o cl_tester cl_tester.c libcsnappy.so
rm -f afifo
mkfifo afifo
LD_LIBRARY_PATH=. ./cl_tester -c <testdata/urls.10K | \
LD_LIBRARY_PATH=. ./cl_tester -d -c > afifo &
diff -u testdata/urls.10K afifo && echo "compress-decompress restores original"
compress-decompress restores original
rm -f afifo
LD_LIBRARY_PATH=. ./cl_tester -S d && echo "decompression is safe"
csnappy_decompress_noheader returned -5.
make: *** [cl_test] Error 1

I think the platform supports unaligned memory access, so I tried the other #if branch in the csnappy_decompress.c, but it fails still with the same error. I don't know what's wrong. The tested function returns CSNAPPY_E_DATA_MALFORMED instead of expected CSNAPPY_E_OUTPUT_OVERRUN there. Just for your info the AArch64 is little-endian.

Every shared library should have a soname for the case when ABI changes and it is necessary to distinguish the incompatible libraries. Otherwise existing executables can start crashing or misbehave.

Please add a soname to the libcsnappy.so library. It's a matter of adding -soname option to the linker and installing proper symlinks to the library. You can also use libtool that helps making the shared library portable.

After getting a report [1] of the Perl module failing on ARM, I tested gsnappy and csnappy on qemu+ARM [2]. gsnappy tested fine, but csnappy did not.

[1] http://www.cpantesters.org/cpan/report/8db4ca7c-6719-11e0-9e8e-799bc13b1876
[2] http://people.debian.org/~aurel32/qemu/arm/

Various files quotes the BSD license, however not all file do so (e.g. the Makefile). Therefore it would nice if you as the author would add a LICENSE file which would declare what license terms applies to the work as as unit.

The latest commit 6c10c30 added check_unaligned_uint64 test which calls "make clean". As a result if tests are performed in parallel (e.g. "make -j4 test"), they fail because the clean target deletes files expected by concurrent jobs.

I've written a Perl interface to csnappy, which is now on CPAN [1]. The earlier version of csnappy compiled under C90 with only a minor tweak, but now it looks like it's going to be quite a chore maintaining a C90-compatible version. Would you consider having csnappy conform to C90? I know your stated goal is for the library to be used in the Linux kernel, but you do have support in the code for non-kernel use, so I'm hoping you'd consider this.

Thanks.

[1] http://search.cpan.org/dist/Compress-Snappy/

Hi,

csnappy has a new binding, see http://github.com/fperrad/lua-csnappy

François

I just pushed out a new version of the Perl module with the latest csnappy source and all the linux tests are coming back with the following error:
src/csnappy_internal_userspace.h:81:3: error: #error __LITTLE_ENDIAN or __BIG_ENDIAN must be defined, not both!

The compiler invocation only ever defines one of those macros:
cc -c -D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -DVERSION="0.09" -DXS_VERSION="0.09" -fPIC "-I/home/cpan/pit/thr/perl-5.13.2/lib/5.13.2/x86_64-linux-thread-multi/CORE" -D__LITTLE_ENDIAN Snappy.c

I only have access to a Mac, which tests fine for me and another tester had no problems with FreeBSD. Tests for the Perl module including more details of the build environments can be found here: http://static.cpantesters.org/distro/C/Compress-Snappy.html#0.09