Hi, Thanks for review. My comments inline.
On 5/18/07, Heikki Orsila <[EMAIL PROTECTED]> wrote:
Good work.. On Fri, May 18, 2007 at 03:28:31PM +0530, Nitin Gupta wrote: > Facts for LZO (at least for original code. Should hold true for this > port also - hence the RFC!): > - The compressor can never overrun buffer. > - The "non-safe" version of decompressor can never overrun buffer if > compressed data is unmodified. I am not sure about this if compressed > data is malicious (to be confirmed from the author). > - The "safe" version can never crash (buffer overrun etc.) - confirmed > from the author. What's the proof?
I confirmned these from the author - I just ported this code. I think he can answer you better - CC'ed him :-)
> +/* LZO1X_1 compression */ > +int > +lzo1x_compress(const unsigned char *src, size_t src_len, > + unsigned char *dst, size_t *dst_len, > + void *workmem); int lzo1x_compress(const unsigned char *src, size_t src_len, unsigned char *dst, size_t *dst_len, void *workmem); is the preferred style.
OK. Changed. <snip>
> + register const unsigned char *ip; Is the register directive really useful? Or any subsequent usage of that directive?
The author must be having some performance gain with this directive. Though I didn't test performance changes with/without this directive.
> + DINDEX1(dindex,ip); Put a space after the delimiter: DINDEX1(dindex, ip); This happens in many places in the source, fix them all.
OK.
Useless brackets: (unsigned char) tt
OK.
> + } > + do *op++ = *ii++; while (--t > 0); memcpy(op, ii, t); ? Happens in other places as well.
I looked more carefully into such cases. Following type of code blocks are repeated at several places: --- COPY4(op,ip); op += 4; ip += 4; if (--t > 0) { if (t >= 4) { do { COPY4(op,ip); op += 4; ip += 4; t -= 4; } while (t >= 4); if (t > 0) do *op++ = *ip++; while (--t > 0); } else do *op++ = *ip++; while (--t > 0); } --- Such entire blocks can be replaced by simple: memcpy(op, ip, t + 4); Since kernel has separate memcpy() implementation optimized for specific archs, we shouldn't loose on perf while having simpler (and shorter) code. I will work on this and post again.
> +#define COPY4(dst,src) *(uint32_t *)(dst) = *(uint32_t *)(src) Use u32.
What is the problem with uint32_t? Anyhow, I think COPY4 will disappear after those memcpy changes :) Thanks for comments. I will post revised patch soon. Cheers, Nitin - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/