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

Sergey Senozhatsky
2013-07-19T09:05:27Z

On (07/19/13 17:27), Kyungsik Lee 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.
>
> How about using "const unsigned char *" for the exported symbols in the
> patch?

will resend shortly.

> It is OK unless the patch changes the internal implementation.
>

it doesn't.

thanks,
-ss