On 5/24/07, Satyam Sharma <satyam.sharma@xxxxxxxxx> wrote:
> [...]
> Just defining and exporting LZO1X_WORKMEM_SIZE may not be enough
> to guarantee that users _will_ pass in workmem of size exactly that much.
>
> If this workmem is really merely a temp buffer required by lzo1x_compress(),
> I'd suggest you rename lzo1x_compress() in lzo1x_compress.c to
> __lzo1x_compress(), and implement a lzo1x_compress() wrapper for the
> user that handles the allocation (and subsequent free'ing) of this temp
> buffer itself.
>
I did not include such wrapper since allocation method will depend on
particular scenario (like kmalloc vs. vmalloc and flags GFP_KERNEL vs
GFP_ATOMIC etc...). But still maybe we can have "default" wrapper that
does, say vmalloc(GFP_KERNEL)/vfree and let others with specific
requirement have their own wrappers.
> [ I vaguely remember discussing something of this sort with Richard
> when he had submitted his patchset too, perhaps you can look into
> his implementation to see how he's managing this ... ]
>
ok.
> > diff --git a/lib/lzo1x/lzo1x_int.h b/lib/lzo1x/lzo1x_int.h
> > [...]
> > +/* Macros for 'safe' decompression */
> > +#ifdef LZO1X_DECOMPRESS_SAFE
> > +
> > +#define lzo1x_decompress lzo1x_decompress_safe
> > +#define TEST_IP (ip < ip_end)
> > +#define NEED_IP(x) \
> > + if ((size_t)(ip_end - ip) < (size_t)(x)) goto input_overrun
> > +#define NEED_OP(x) \
> > + if ((size_t)(op_end - op) < (size_t)(x)) goto output_overrun
> > +#define TEST_LB(m_pos) \
> > + if (m_pos < out || m_pos >= op) goto lookbehind_overrun
> > +#define HAVE_TEST_IP
> > +#define HAVE_ANY_OP
> > +
> > +#else /* !LZO1X_DECOMPRESS_SAFE */
> > +
> > +#define TEST_IP 1
> > +#define TEST_LB(x) ((void) 0)
> > +#define NEED_IP(x) ((void) 0)
> > +#define NEED_OP(x) ((void) 0)
> > +#undef HAVE_TEST_IP
> > +#undef HAVE_ANY_OP
> > +
> > +#endif /* LZO1X_DECOMPRESS_SAFE */
>
> ... ugh. Yes, extracting the common stuff between the _safe and _unsafe
> variants in a common low-level __lzo1x_decompress kind of function
> definitely looks doable. The low-level function could simply take an extra
> argument (say, set by the _safe and _unsafe wrappers) that tells it
> whether it is being called as safe or unsafe ... helps us get rid of the
> disruptions to all the Makefiles above and these #ifdef's ugliness ..
Hmm..I will try to get this done.
> BTW, it'd be really cool if Richard and yourself could get together and
> pool your energies / efforts to develop a common / same patchset for this.
> (I wonder how different your implementations are, actually, and if there
> are any significant performance disparities, especially.) I really like your
> work, as it clears up the major gripe I had with Richard's patchset -- the
> ugliness (and monstrosity) of it.
I am really looking forward to co-operating with Richard regarding
this - although our approach for this porting is quite different but I
hope we can get around this. Duplication sucks! :)