From 88605795edd87da92e48518d2d7c5c21e21200da Mon Sep 17 00:00:00 2001 From: Vianney Tran Date: Wed, 14 Mar 2018 10:46:06 +0100 Subject: [PATCH 1/4] [zstd][errors] Get rid of static error code translation and use C call - Errors were defined in private C/header files and we needed to change them every release (FIXME: this is very fragile, must map 1-to-1 with zstd's) - We actually just need DstSizeTooSmall error which we now have a check for and a test --- errors.go | 34 ++++++++++++++++++++++++++++++ errors_test.go | 24 ++++++++++++++++++++++ zstd.go | 56 ++------------------------------------------------ 3 files changed, 60 insertions(+), 54 deletions(-) create mode 100644 errors.go create mode 100644 errors_test.go diff --git a/errors.go b/errors.go new file mode 100644 index 0000000..840294a --- /dev/null +++ b/errors.go @@ -0,0 +1,34 @@ +package zstd + +/* +#define ZSTD_STATIC_LINKING_ONLY +#include "zstd.h" +*/ +import "C" + +// ErrorCode is an error returned by the zstd library. +type ErrorCode int + +func (e ErrorCode) Error() string { + a := C.ZSTD_getErrorName(C.size_t(e)) + return C.GoString(a) +} + +func cIsError(code int) bool { + return int(C.ZSTD_isError(C.size_t(code))) != 0 +} + +// getError returns an error for the return code, or nil if it's not an error +func getError(code int) error { + if code < 0 && cIsError(code) { + return ErrorCode(code) + } + return nil +} + +func IsDstSizeTooSmallError(e error) bool { + if e != nil && e.Error() == "Destination buffer is too small" { + return true + } + return false +} diff --git a/errors_test.go b/errors_test.go new file mode 100644 index 0000000..9f5187f --- /dev/null +++ b/errors_test.go @@ -0,0 +1,24 @@ +package zstd + +import ( + "testing" +) + +const ( + // ErrorUpperBound is the upper bound to error number, currently only used in test + // If this needs to be updated, check in zstd_errors.h what the max is + ErrorUpperBound = 1000 +) + +// TestFindIsDstSizeTooSmallError tests that there is at least one error code that +// corresponds to dst size too small +func TestFindIsDstSizeTooSmallError(t *testing.T) { + for i := -1; i > -ErrorUpperBound; i-- { + e := ErrorCode(i) + if IsDstSizeTooSmallError(e) { + return // Found + } + } + + t.Fatal("Couldn't find an error code for DstSizeTooSmall error, please make sure we didn't change the error string") +} diff --git a/zstd.go b/zstd.go index 9ff60d6..0154ede 100644 --- a/zstd.go +++ b/zstd.go @@ -12,53 +12,13 @@ import ( "unsafe" ) -// ErrorCode is an error returned by the zstd library. -type ErrorCode int - -func (e ErrorCode) Error() string { - if C.ZSTD_isError(C.size_t(e)) != C.uint(0) { - return C.GoString(C.ZSTD_getErrorName(C.size_t(e))) - } - - return "" -} - const ( BestSpeed = 1 BestCompression = 20 ) -// FIXME: this is very fragile, must map 1-to-1 with zstd's -// error_public.h. They have no problem with adding error codes, -// renumbering errors, etc. var ( - ErrGeneric ErrorCode = -1 - ErrPrefixUnknown ErrorCode = -2 - ErrVersionUnsupported ErrorCode = -3 - ErrParameterUnknown ErrorCode = -4 - ErrFrameParameterUnsupported ErrorCode = -5 - ErrFrameParameterUnsupportedBy32bits ErrorCode = -6 - ErrFrameParameterWindowTooLarge ErrorCode = -7 - ErrCompressionParameterUnsupported ErrorCode = -8 - ErrCompressionParameterOutOfBound ErrorCode = -9 - ErrInitMissing ErrorCode = -10 - ErrMemoryAllocation ErrorCode = -11 - ErrStageWrong ErrorCode = -12 - ErrDstSizeTooSmall ErrorCode = -13 - ErrSrcSizeWrong ErrorCode = -14 - ErrCorruptionDetected ErrorCode = -15 - ErrChecksumWrong ErrorCode = -16 - ErrTableLogTooLarge ErrorCode = -17 - ErrMaxSymbolValueTooLarge ErrorCode = -18 - ErrMaxSymbolValueTooSmall ErrorCode = -19 - ErrDictionaryCorrupted ErrorCode = -20 - ErrDictionaryWrong ErrorCode = -21 - ErrDictionaryCreationFailed ErrorCode = -22 - ErrFrameIndexTooLarge ErrorCode = -23 - ErrSeekableIO ErrorCode = -24 - ErrMaxCode ErrorCode = -25 - ErrEmptySlice = errors.New("Bytes slice is empty") - + ErrEmptySlice = errors.New("Bytes slice is empty") DefaultCompression = 5 ) @@ -79,18 +39,6 @@ func cCompressBound(srcSize int) int { return int(C.ZSTD_compressBound(C.size_t(srcSize))) } -// getError returns an error for the return code, or nil if it's not an error -func getError(code int) error { - if code < 0 && code > int(ErrMaxCode) { - return ErrorCode(code) - } - return nil -} - -func cIsError(code int) bool { - return int(C.ZSTD_isError(C.size_t(code))) != 0 -} - // Compress src into dst. If you have a buffer to use, you can pass it to // prevent allocation. If it is too small, or if nil is passed, a new buffer // will be allocated and returned. @@ -161,7 +109,7 @@ func Decompress(dst, src []byte) ([]byte, error) { } for i := 0; i < 3; i++ { // 3 tries to allocate a bigger buffer result, err := decompress(dst, src) - if err != ErrDstSizeTooSmall { + if !IsDstSizeTooSmallError(err) { return result, err } dst = make([]byte, len(dst)*2) // Grow buffer by 2 From 039e6f59664ad4509cf3eb8cc9ea3391010116f3 Mon Sep 17 00:00:00 2001 From: Vianney Tran Date: Wed, 14 Mar 2018 11:24:51 +0100 Subject: [PATCH 2/4] [errors] nit: remove intermediate variable --- errors.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/errors.go b/errors.go index 840294a..49ae104 100644 --- a/errors.go +++ b/errors.go @@ -9,9 +9,9 @@ import "C" // ErrorCode is an error returned by the zstd library. type ErrorCode int +// Error returns the error string given by zstd func (e ErrorCode) Error() string { - a := C.ZSTD_getErrorName(C.size_t(e)) - return C.GoString(a) + return C.GoString(C.ZSTD_getErrorName(C.size_t(e))) } func cIsError(code int) bool { From 00c3d53271e9052af4bf955b269ddca62af68ef3 Mon Sep 17 00:00:00 2001 From: Vianney Tran Date: Wed, 14 Mar 2018 11:56:14 +0100 Subject: [PATCH 3/4] [tests][errors] Make TestFindIsDstSizeTooSmallError stricter TestFindIsDstSizeTooSmallError now check that there is ONE AND ONLY ONE zstd error that corresponds to DstSizeTooSmall error to make sure the check is not too leniant (before just tested we had at least one) --- errors_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/errors_test.go b/errors_test.go index 9f5187f..ce661fc 100644 --- a/errors_test.go +++ b/errors_test.go @@ -13,12 +13,17 @@ const ( // TestFindIsDstSizeTooSmallError tests that there is at least one error code that // corresponds to dst size too small func TestFindIsDstSizeTooSmallError(t *testing.T) { + found := 0 for i := -1; i > -ErrorUpperBound; i-- { e := ErrorCode(i) if IsDstSizeTooSmallError(e) { - return // Found + found++ } } - t.Fatal("Couldn't find an error code for DstSizeTooSmall error, please make sure we didn't change the error string") + if found == 0 { + t.Fatal("Couldn't find an error code for DstSizeTooSmall error, please make sure we didn't change the error string") + } else if found > 1 { + t.Fatal("IsDstSizeTooSmallError found multiple error codes matching, this shouldn't be the case") + } } From 3b90611f163b574ec32264f5887094f3c3ee1254 Mon Sep 17 00:00:00 2001 From: Vianney Tran Date: Wed, 14 Mar 2018 11:57:33 +0100 Subject: [PATCH 4/4] [zstd] Linting Add comments to exported variables and methods --- errors.go | 1 + zstd.go | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/errors.go b/errors.go index 49ae104..38db0d5 100644 --- a/errors.go +++ b/errors.go @@ -26,6 +26,7 @@ func getError(code int) error { return nil } +// IsDstSizeTooSmallError returns whether the error correspond to zstd standard sDstSizeTooSmall error func IsDstSizeTooSmallError(e error) bool { if e != nil && e.Error() == "Destination buffer is too small" { return true diff --git a/zstd.go b/zstd.go index 0154ede..56f215a 100644 --- a/zstd.go +++ b/zstd.go @@ -12,14 +12,16 @@ import ( "unsafe" ) +// Defines best and standard values for zstd cli const ( - BestSpeed = 1 - BestCompression = 20 + BestSpeed = 1 + BestCompression = 20 + DefaultCompression = 5 ) var ( - ErrEmptySlice = errors.New("Bytes slice is empty") - DefaultCompression = 5 + // ErrEmptySlice is returned when there is nothing to compress + ErrEmptySlice = errors.New("Bytes slice is empty") ) // CompressBound returns the worst case size needed for a destination buffer,