lkml.org 
[lkml]   [2007]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2007-05-18 13:29    [from the cache]
©2003-2011 Jasper Spaans