Re: [PATCH v18 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov <dplotnikov@virtuozzo.com>]

On 16.04.2020 15:55, Alberto Garcia wrote:
> On Thu 02 Apr 2020 08:36:44 AM CEST, Denis Plotnikov wrote:
>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>> +                                   const void *src, size_t src_size)
>> +{
>> +    ssize_t ret;
>> +    ZSTD_outBuffer output = { dest, dest_size, 0 };
>> +    ZSTD_inBuffer input = { src, src_size, 0 };
>> +    ZSTD_CCtx *cctx = ZSTD_createCCtx();
>> +
>> +    if (!cctx) {
>> +        return -EIO;
>> +    }
>> +    /*
>> +     * Use the zstd streamed interface for symmetry with decompression,
>> +     * where streaming is essential since we don't record the exact
>> +     * compressed size.
>> +     *
>> +     * In the loop, we try to compress all the data into one zstd frame.
>> +     * ZSTD_compressStream2 potentially can finish a frame earlier
>> +     * than the full input data is consumed. That's why we are looping
>> +     * until all the input data is consumed.
>> +     */
>> +    while (input.pos < input.size) {
>> +        size_t zstd_ret;
>> +        /*
>> +         * ZSTD spec: "You must continue calling ZSTD_compressStream2()
>> +         * with ZSTD_e_end until it returns 0, at which point you are
>> +         * free to start a new frame". We assume that "start a new frame"
>> +         * means call ZSTD_compressStream2 in the very beginning or when
>> +         * ZSTD_compressStream2 has returned with 0.
>> +         */
>> +        do {
>> +            zstd_ret = ZSTD_compressStream2(cctx, &output, &input, ZSTD_e_end);
>> +
>> +            if (ZSTD_isError(zstd_ret)) {
>> +                ret = -EIO;
>> +                goto out;
>> +            }
>> +            /* Dest buffer isn't big enough to store compressed content */
>> +            if (zstd_ret > output.size - output.pos) {
>> +                ret = -ENOMEM;
>> +                goto out;
>> +            }
>> +        } while (zstd_ret);
>> +    }
>> +    /* make sure we can safely return compressed buffer size with ssize_t */
>> +    assert(output.pos <= SSIZE_MAX);
> The patch looks good to me, but why don't you assert this at the
> beginning of the function? You already know the buffer sizes...
Yes, that's true. But I thought that it's reasonable to check what is 
returned.
"output.pos" could be less then or equal to ssize_max when output.size > 
ssize_max.
Anyway, this case most probably won't exist, and this is just an 
overflow inssurance.
>
> Either way
>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
>
> Berto
Thanks for reviewing!

Denis