Re: [PATCH v5 4/5] qcow2: add zstd cluster compression

[Alberto Garcia]

On Wed 04 Mar 2020 02:35:37 PM CET, Denis Plotnikov wrote:
> +#ifdef CONFIG_ZSTD
> +
> +#define ZSTD_LEN_BUF 4

I think it's worth adding a comment explaining what this is. I know it's
quite clear once you read the code, but still...

> +/*
> + * qcow2_zstd_decompress()
> + *
> + * Decompress some data (not more than @src_size bytes) to produce exactly
> + * @dest_size bytes using zstd compression method
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + *          -EIO on any error
> + */
> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
> +                                     const void *src, size_t src_size)
> +{
> +    /*
> +     * zstd decompress wants to know the exact length of the data.
> +     * For that purpose, on compression, the length is stored in
> +     * the very beginning of the compressed buffer
> +     */
> +    size_t s_size;
> +    const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
> +
> +    /*
> +     * sanity check that we can read 4 byte the content length and
> +     * and there is some content to decompress
> +     */
> +    if (src_size <= ZSTD_LEN_BUF) {
> +        return -EIO;
> +    }
> +
> +    s_size = ldl_be_p(src);
> +
> +    /* sanity check that the buffer is big enough to read the content from */
> +    if (src_size - ZSTD_LEN_BUF < s_size) {
> +        return -EIO;
> +    }
> +
> +    if (ZSTD_isError(
> +            ZSTD_decompress(dest, dest_size, s_buf, s_size))) {
> +        return -EIO;
> +    }
> +
> +    return 0;
> +}

In this one you could also return -ENOMEM if the destination buffer is
not big enough.

But not of my comments is so important, so whether you decide to make
those changes or not,

Reviewed-by: Alberto Garcia <address@hidden>

Berto