Re: [PATCH] LZ4: compression/decompression signedness mismatch

Geert Uytterhoeven
2013-07-18T21:29:59Z

On Thu, Jul 18, 2013 at 11:18 PM, Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
> On (07/12/13 12:48), Sergey Senozhatsky wrote:
>> On (07/12/13 11:28), Yann Collet wrote:
>> > The reference implementation, hosted at :�
>> > [1]https://code.google.com/p/lz4/
>> > only proposes char* (signed) types as part of the interface contract.
>> > I would recommend to keep it that way, to remain consistent.
>> > Regards
>>
>> Crypto lz4 accepts u8 * for both compression and decompression:
>>
>> lz4_compress_crypto(struct crypto_tfm *tfm, const u8 *src,
>> unsigned int slen, u8 *dst, unsigned int *dlen)
>>
>> lz4_decompress_crypto(struct crypto_tfm *tfm, const u8 *src,
>> unsigned int slen, u8 *dst, unsigned int *dlen)
>>
>>
>> Internally LZ4 may cast unsigned char* to signed char*, the same way you
>> already do with compression:
>>
>> int lz4_compress(const unsigned char *src, size_t src_len,
>> unsigned char *dst, size_t *dst_len, void *wrkmem)
>>
>> calls:
>> lz4_compressctx(void *ctx,
>> const char *source, char *dest,
>> int isize, int maxoutputsize)
>>
>
>
> + lib/decompress_unlz4.c
> STATIC int INIT decompress(unsigned char *buf, int in_len,
> int(*fill)(void*, unsigned int),
> int(*flush)(void*, unsigned int),
> unsigned char *output,
> int *posp,
> void(*error)(char *x)
>
>>
>> At the moment API is a bit misaligned: unsiged char* for compression and signed char* for
>> decompression.
>>
>>
>> My 'real word' use case is, suppose:
>>
>> struct foo {
>> [..]
>> int (*compress)(const unsigned char *src, size_t src_len,
>> unsigned char *dst, size_t *dst_len, void *wrkmem);
>> int (*decompress)(const unsigned char *src, size_t src_len,
>> unsigned char *dst, size_t *dst_len);
>> };
>>
>>
>> and (for example) module also provides sysfs attribute, so user can switch select
>> LZO or LZ4 compressions depending of his needs:
>>
>> ->compress = lzo1x_1_compress;
>> ->decompress = lzo1x_decompress_safe;
>>
>> to
>> ->compress = lz4_compress;
>> ->decompress = lz4_decompress_unknownoutputsize;
>>
>>
>> the last one produces unneccessary compilation warning.
>>
>
> did you guys have a chance to review the patch? it does not change
> implementation/internals, just decompression exported symbols.

IMHO, all these memory buffers should be of type "(const) void *", cfr.
e.g. read(2) and memcpy(3).

This avoids casts in the callers.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds