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

[Kyungsik Lee]

On Thu, Jul 18, 2013 at 11:29:57PM +0200, Geert Uytterhoeven wrote:
> On Thu, Jul 18, 2013 at 11:18 PM, Sergey Senozhatsky
> <sergey.senozhatsky@xxxxxxxxx> 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.

How about using "const unsigned char *" for the exported symbols in the
patch?
It is OK unless the patch changes the internal implementation.

Thanks,
Kyungsik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/