Re: [RFC] LZO1X de/compression support

[Nitin Gupta]

Hi,

Thanks for review. My comments inline.

On 5/18/07, Heikki Orsila <shdl@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/