Messages in this thread | | | Date | Fri, 18 May 2007 16:57:25 +0530 | From | "Nitin Gupta" <> | Subject | Re: [RFC] LZO1X de/compression support |
| |
Hi,
Thanks for review. My comments inline.
On 5/18/07, Heikki Orsila <shdl@zakalwe.fi> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |