[systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor

[Zbigniew Jędrzejewski-Szmek]

On Sun, Jul 06, 2014 at 09:47:02PM +0200, Lennart Poettering wrote:
> On Sat, 05.07.14 20:56, Zbigniew Jędrzejewski-Szmek (zbyszek at in.waw.pl) wrote:
> 
> > -bool uncompress_blob(const void *src, uint64_t src_size,
> > -                     void **dst, uint64_t *dst_alloc_size, uint64_t* dst_size, uint64_t dst_max) {
> > +int compress_blob_lz4(const void *src, uint64_t src_size, void *dst, uint64_t *dst_size) {
> > +#ifdef HAVE_LZ4
> > +        int r;
> >  
> > +        assert(src);
> > +        assert(src_size > 0);
> > +        assert(dst);
> > +        assert(dst_size);
> > +
> > +        /* Returns false if we couldn't compress the data or the
> > +         * compressed result is longer than the original */
> > +
> > +        if (src_size < 9)
> > +                return -ENOSPC;
> > +
> > +        r = LZ4_compress_limitedOutput(src, dst + 8, src_size, src_size - 8 - 1);
> > +        if (r <= 0)
> > +                return -ENOSPC;
> 
> Maybe EAGAIN? Would feel a bit like a kernel API then ;-).
I think it would be confusing... But I see now that there's
EFBIG and ENOBUFS, which seem to fit much better: file to big, no space in buffer.

> 
> > +        buf1 = malloc(LZ4_BUFSIZE);
> > +        buf2 = malloc(LZ4_BUFSIZE);
> > +        out = malloc(LZ4_COMPRESSBOUND(LZ4_BUFSIZE));
> > +        if (!buf1 || !buf2 || !out)
> > +                return log_oom();
> 
> Shouldn't we just return -ENOMEM here? Feels like a "library" function, not
> a "program" function, hence it should not log at the anything but debug, really....
> 
> > +        out = malloc(4*LZ4_BUFSIZE);
> > +        if (!out)
> > +                return log_oom();
> 
> Same here...
This is in the "fd streaming" code, which outputs error messages on its own. They
could be moved to the caller, but code would get more complicated with little gain.
I'll keep it this way for now, since there's only one caller and it doesn't really
matter.

> Looks good otheriwse.
> 
> BTW, have you checked whether reuseing the XZ context might make the XZ
> more competitive?
No, I haven't. But I seriously doubt how it could improve things enough to
make a difference (> 2 orders).

Zbyszek