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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
