Re: [RFC] LZO1X de/compression support

["Nitin Gupta" <nitingupta910@gmail.com>]

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