qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Denis Plotnikov
Subject: Re: [PATCH v18 3/4] qcow2: add zstd cluster compression
Date: Thu, 16 Apr 2020 16:07:55 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1



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 <address@hidden>

Berto
Thanks for reviewing!

Denis




reply via email to

[Prev in Thread] Current Thread [Next in Thread]