[PATCH v2 0/4] luks: add qemu-img measure support

[PATCH v2 0/4] luks: add qemu-img measure support

[Stefan Hajnoczi]

v2:
 * Fix uint64_t <-> size_t type mismatch in block_crypto_measure() so that
   32-bit builds pass

This patch series adds qemu-img measure support to the "luks" block driver.  We
just need to take into account the LUKS header when sizing the image.

Stefan Hajnoczi (4):
  luks: extract block_crypto_calculate_payload_offset()
  luks: implement .bdrv_measure()
  qemu-img: allow qemu-img measure --object without a filename
  iotests: add 282 luks qemu-img measure test

 block/crypto.c             | 146 +++++++++++++++++++++++++++++++++++++
 block/crypto.h             |   5 ++
 block/qcow2.c              |  59 +--------------
 qemu-img.c                 |   6 +-
 tests/qemu-iotests/282     |  93 +++++++++++++++++++++++
 tests/qemu-iotests/282.out |  30 ++++++++
 tests/qemu-iotests/group   |   1 +
 7 files changed, 281 insertions(+), 59 deletions(-)
 create mode 100755 tests/qemu-iotests/282
 create mode 100644 tests/qemu-iotests/282.out

-- 
2.24.1

[PATCH v2 1/4] luks: extract block_crypto_calculate_payload_offset()

[Stefan Hajnoczi]

The qcow2 .bdrv_measure() code calculates the crypto payload offset.
This logic really belongs in block/crypto.c where it can be reused by
other image formats.

The "luks" block driver will need this same logic in order to implement
.bdrv_measure(), so extract the block_crypto_calculate_payload_offset()
function now.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/crypto.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/crypto.h |  5 ++++
 block/qcow2.c  | 59 ++++------------------------------------------
 3 files changed, 73 insertions(+), 55 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 24823835c1..ed32202fa2 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -185,6 +185,70 @@ block_crypto_create_opts_init(QDict *opts, Error **errp)
 }
 
 
+static ssize_t block_crypto_headerlen_hdr_init_func(QCryptoBlock *block,
+        size_t headerlen, void *opaque, Error **errp)
+{
+    size_t *headerlenp = opaque;
+
+    /* Stash away the payload size */
+    *headerlenp = headerlen;
+    return 0;
+}
+
+
+static ssize_t block_crypto_headerlen_hdr_write_func(QCryptoBlock *block,
+        size_t offset, const uint8_t *buf, size_t buflen,
+        void *opaque, Error **errp)
+{
+    /* Discard the bytes, we're not actually writing to an image */
+    return buflen;
+}
+
+
+/* Determine the number of bytes for the crypto header */
+bool block_crypto_calculate_payload_offset(QemuOpts *opts,
+                                           const char *optprefix,
+                                           size_t *len,
+                                           Error **errp)
+{
+    QDict *cryptoopts_qdict;
+    QCryptoBlockCreateOptions *cryptoopts;
+    QCryptoBlock *crypto;
+
+    /* Extract options into a qdict */
+    if (optprefix) {
+        QDict *opts_qdict = qemu_opts_to_qdict(opts, NULL);
+
+        qdict_extract_subqdict(opts_qdict, &cryptoopts_qdict, optprefix);
+        qobject_unref(opts_qdict);
+    } else {
+        cryptoopts_qdict = qemu_opts_to_qdict(opts, NULL);
+    }
+
+    /* Build QCryptoBlockCreateOptions object from qdict */
+    qdict_put_str(cryptoopts_qdict, "format", "luks");
+
+    cryptoopts = block_crypto_create_opts_init(cryptoopts_qdict, errp);
+    qobject_unref(cryptoopts_qdict);
+    if (!cryptoopts) {
+        return false;
+    }
+
+    /* Fake LUKS creation in order to determine the payload size */
+    crypto = qcrypto_block_create(cryptoopts, optprefix,
+                                  block_crypto_headerlen_hdr_init_func,
+                                  block_crypto_headerlen_hdr_write_func,
+                                  len, errp);
+    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
+    if (!crypto) {
+        return false;
+    }
+
+    qcrypto_block_free(crypto);
+    return true;
+}
+
+
 static int block_crypto_open_generic(QCryptoBlockFormat format,
                                      QemuOptsList *opts_spec,
                                      BlockDriverState *bs,
diff --git a/block/crypto.h b/block/crypto.h
index b935695e79..5a39bee71b 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -94,4 +94,9 @@ block_crypto_create_opts_init(QDict *opts, Error **errp);
 QCryptoBlockOpenOptions *
 block_crypto_open_opts_init(QDict *opts, Error **errp);
 
+bool block_crypto_calculate_payload_offset(QemuOpts *opts,
+                                           const char *optprefix,
+                                           size_t *len,
+                                           Error **errp);
+
 #endif /* BLOCK_CRYPTO_H */
diff --git a/block/qcow2.c b/block/qcow2.c
index cef9d72b3a..3aaf79bbf6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4609,60 +4609,6 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
     return ret;
 }
 
-static ssize_t qcow2_measure_crypto_hdr_init_func(QCryptoBlock *block,
-        size_t headerlen, void *opaque, Error **errp)
-{
-    size_t *headerlenp = opaque;
-
-    /* Stash away the payload size */
-    *headerlenp = headerlen;
-    return 0;
-}
-
-static ssize_t qcow2_measure_crypto_hdr_write_func(QCryptoBlock *block,
-        size_t offset, const uint8_t *buf, size_t buflen,
-        void *opaque, Error **errp)
-{
-    /* Discard the bytes, we're not actually writing to an image */
-    return buflen;
-}
-
-/* Determine the number of bytes for the LUKS payload */
-static bool qcow2_measure_luks_headerlen(QemuOpts *opts, size_t *len,
-                                         Error **errp)
-{
-    QDict *opts_qdict;
-    QDict *cryptoopts_qdict;
-    QCryptoBlockCreateOptions *cryptoopts;
-    QCryptoBlock *crypto;
-
-    /* Extract "encrypt." options into a qdict */
-    opts_qdict = qemu_opts_to_qdict(opts, NULL);
-    qdict_extract_subqdict(opts_qdict, &cryptoopts_qdict, "encrypt.");
-    qobject_unref(opts_qdict);
-
-    /* Build QCryptoBlockCreateOptions object from qdict */
-    qdict_put_str(cryptoopts_qdict, "format", "luks");
-    cryptoopts = block_crypto_create_opts_init(cryptoopts_qdict, errp);
-    qobject_unref(cryptoopts_qdict);
-    if (!cryptoopts) {
-        return false;
-    }
-
-    /* Fake LUKS creation in order to determine the payload size */
-    crypto = qcrypto_block_create(cryptoopts, "encrypt.",
-                                  qcow2_measure_crypto_hdr_init_func,
-                                  qcow2_measure_crypto_hdr_write_func,
-                                  len, errp);
-    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
-    if (!crypto) {
-        return false;
-    }
-
-    qcrypto_block_free(crypto);
-    return true;
-}
-
 static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
                                        Error **errp)
 {
@@ -4715,7 +4661,10 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
     if (has_luks) {
         size_t headerlen;
 
-        if (!qcow2_measure_luks_headerlen(opts, &headerlen, &local_err)) {
+        if (!block_crypto_calculate_payload_offset(opts,
+                                                   "encrypt.",
+                                                   &headerlen,
+                                                   &local_err)) {
             goto err;
         }
 
-- 
2.24.1

[PATCH v2 2/4] luks: implement .bdrv_measure()

[Stefan Hajnoczi]

Add qemu-img measure support in the "luks" block driver.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/crypto.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index ed32202fa2..51f37bb1f6 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -548,6 +548,87 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
 }
 
 
+static BlockMeasureInfo *block_crypto_measure(QemuOpts *opts,
+                                              BlockDriverState *in_bs,
+                                              Error **errp)
+{
+    Error *local_err = NULL;
+    BlockMeasureInfo *info;
+    uint64_t required = 0; /* bytes that contribute to required size */
+    uint64_t virtual_size; /* disk size as seen by guest */
+    size_t luks_payload_size;
+    char *optstr;
+    PreallocMode prealloc;
+
+    optstr = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
+    prealloc = qapi_enum_parse(&PreallocMode_lookup, optstr,
+                               PREALLOC_MODE_OFF, &local_err);
+    g_free(optstr);
+    if (local_err) {
+        goto err;
+    }
+
+    virtual_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+
+    if (!block_crypto_calculate_payload_offset(opts, NULL, &luks_payload_size,
+                                               &local_err)) {
+        goto err;
+    }
+
+    if (in_bs) {
+        int64_t ssize;
+        int64_t offset;
+        int64_t pnum = 0;
+
+        ssize = bdrv_getlength(in_bs);
+        if (ssize < 0) {
+            error_setg_errno(&local_err, -ssize,
+                             "Unable to get image virtual_size");
+            goto err;
+        }
+
+        virtual_size = ssize;
+
+        for (offset = 0; offset < ssize; offset += pnum) {
+            int ret;
+
+            ret = bdrv_block_status_above(in_bs, NULL, offset,
+                                          ssize - offset, &pnum, NULL,
+                                          NULL);
+            if (ret < 0) {
+                error_setg_errno(&local_err, -ret,
+                                 "Unable to get block status");
+                goto err;
+            }
+
+            if (ret & BDRV_BLOCK_ZERO) {
+                /* Skip zero regions */
+            } else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
+                       (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) {
+                /* Count clusters we've seen */
+                required += pnum;
+            }
+        }
+    }
+
+    /* Take into account preallocation.  Nothing special is needed for
+     * PREALLOC_MODE_METADATA since metadata is always counted.
+     */
+    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
+        required = virtual_size;
+    }
+
+    info = g_new(BlockMeasureInfo, 1);
+    info->fully_allocated = luks_payload_size + virtual_size;
+    info->required = luks_payload_size + required;
+    return info;
+
+err:
+    error_propagate(errp, local_err);
+    return NULL;
+}
+
+
 static int block_crypto_probe_luks(const uint8_t *buf,
                                    int buf_size,
                                    const char *filename) {
@@ -734,6 +815,7 @@ static BlockDriver bdrv_crypto_luks = {
     .bdrv_co_preadv     = block_crypto_co_preadv,
     .bdrv_co_pwritev    = block_crypto_co_pwritev,
     .bdrv_getlength     = block_crypto_getlength,
+    .bdrv_measure       = block_crypto_measure,
     .bdrv_get_info      = block_crypto_get_info_luks,
     .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
 
-- 
2.24.1

[PATCH v2 3/4] qemu-img: allow qemu-img measure --object without a filename

[Stefan Hajnoczi]

In most qemu-img sub-commands the --object option only makes sense when
there is a filename.  qemu-img measure is an exception because objects
may be referenced from the image creation options instead of an existing
image file.  Allow --object without a filename.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6233b8ca56..4f3e58f13b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4903,10 +4903,8 @@ static int img_measure(int argc, char **argv)
         filename = argv[optind];
     }
 
-    if (!filename &&
-        (object_opts || image_opts || fmt || snapshot_name || sn_opts)) {
-        error_report("--object, --image-opts, -f, and -l "
-                     "require a filename argument.");
+    if (!filename && (image_opts || fmt || snapshot_name || sn_opts)) {
+        error_report("--image-opts, -f, and -l require a filename argument.");
         goto out;
     }
     if (filename && img_size != UINT64_MAX) {
-- 
2.24.1

[PATCH v2 4/4] iotests: add 282 luks qemu-img measure test

[Stefan Hajnoczi]

This test exercises the block/crypto.c "luks" block driver
.bdrv_measure() code.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/282     | 93 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/282.out | 30 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 124 insertions(+)
 create mode 100755 tests/qemu-iotests/282
 create mode 100644 tests/qemu-iotests/282.out

diff --git a/tests/qemu-iotests/282 b/tests/qemu-iotests/282
new file mode 100755
index 0000000000..6c62065aef
--- /dev/null
+++ b/tests/qemu-iotests/282
@@ -0,0 +1,93 @@
+#!/usr/bin/env bash
+#
+# qemu-img measure tests for LUKS images
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=stefanha@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1    # failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "$TEST_IMG.converted"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt luks
+_supported_proto file
+_supported_os Linux
+
+SECRET=secret,id=sec0,data=passphrase
+
+echo "== measure 1G image file =="
+echo
+
+$QEMU_IMG measure --object "$SECRET" \
+	          -O "$IMGFMT" \
+		  -o key-secret=sec0,iter-time=10 \
+		  --size 1G
+
+echo
+echo "== create 1G image file (size should be no greater than measured) =="
+echo
+
+_make_test_img 1G
+stat -c "image file size in bytes: %s" "$TEST_IMG_FILE"
+
+echo
+echo "== modified 1G image file (size should be no greater than measured) =="
+echo
+
+$QEMU_IO --object "$SECRET" --image-opts "$TEST_IMG" -c "write -P 0x51 0x10000 0x400" | _filter_qemu_io | _filter_testdir
+stat -c "image file size in bytes: %s" "$TEST_IMG_FILE"
+
+echo
+echo "== measure preallocation=falloc 1G image file =="
+echo
+
+$QEMU_IMG measure --object "$SECRET" \
+	          -O "$IMGFMT" \
+		  -o key-secret=sec0,iter-time=10,preallocation=falloc \
+		  --size 1G
+
+echo
+echo "== measure with input image file =="
+echo
+
+IMGFMT=raw IMGKEYSECRET= IMGOPTS= _make_test_img 1G | _filter_imgfmt
+QEMU_IO_OPTIONS= IMGOPTSSYNTAX= $QEMU_IO -f raw -c "write -P 0x51 0x10000 0x400" "$TEST_IMG_FILE" | _filter_qemu_io | _filter_testdir
+$QEMU_IMG measure --object "$SECRET" \
+	          -O "$IMGFMT" \
+		  -o key-secret=sec0,iter-time=10 \
+		  -f raw \
+		  "$TEST_IMG_FILE"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/282.out b/tests/qemu-iotests/282.out
new file mode 100644
index 0000000000..8f4737aef7
--- /dev/null
+++ b/tests/qemu-iotests/282.out
@@ -0,0 +1,30 @@
+QA output created by 282
+== measure 1G image file ==
+
+required size: 2068480
+fully allocated size: 1075810304
+
+== create 1G image file (size should be no greater than measured) ==
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+image file size in bytes: 1075810304
+
+== modified 1G image file (size should be no greater than measured) ==
+
+wrote 1024/1024 bytes at offset 65536
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+image file size in bytes: 1075810304
+
+== measure preallocation=falloc 1G image file ==
+
+required size: 1075810304
+fully allocated size: 1075810304
+
+== measure with input image file ==
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+wrote 1024/1024 bytes at offset 65536
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+required size: 2076672
+fully allocated size: 1075810304
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cb2b789e44..d114e2c971 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -288,3 +288,4 @@
 277 rw quick
 279 rw backing quick
 280 rw migration quick
+282 quick
-- 
2.24.1

Re: [PATCH v2 1/4] luks: extract block_crypto_calculate_payload_offset()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1860 bytes --]

On 09.01.20 12:10, Stefan Hajnoczi wrote:
> The qcow2 .bdrv_measure() code calculates the crypto payload offset.
> This logic really belongs in block/crypto.c where it can be reused by
> other image formats.
> 
> The "luks" block driver will need this same logic in order to implement
> .bdrv_measure(), so extract the block_crypto_calculate_payload_offset()
> function now.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/crypto.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/crypto.h |  5 ++++
>  block/qcow2.c  | 59 ++++------------------------------------------
>  3 files changed, 73 insertions(+), 55 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 24823835c1..ed32202fa2 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -185,6 +185,70 @@ block_crypto_create_opts_init(QDict *opts, Error **errp)

[...]

> +/* Determine the number of bytes for the crypto header */
> +bool block_crypto_calculate_payload_offset(QemuOpts *opts,
> +                                           const char *optprefix,
> +                                           size_t *len,
> +                                           Error **errp)
> +{
> +    QDict *cryptoopts_qdict;
> +    QCryptoBlockCreateOptions *cryptoopts;
> +    QCryptoBlock *crypto;
> +
> +    /* Extract options into a qdict */
> +    if (optprefix) {
> +        QDict *opts_qdict = qemu_opts_to_qdict(opts, NULL);
> +
> +        qdict_extract_subqdict(opts_qdict, &cryptoopts_qdict, optprefix);
> +        qobject_unref(opts_qdict);
> +    } else {
> +        cryptoopts_qdict = qemu_opts_to_qdict(opts, NULL);
> +    }
> +
> +    /* Build QCryptoBlockCreateOptions object from qdict */
> +    qdict_put_str(cryptoopts_qdict, "format", "luks");

Should this be a parameter?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] luks: implement .bdrv_measure()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1453 bytes --]

On 09.01.20 12:10, Stefan Hajnoczi wrote:
> Add qemu-img measure support in the "luks" block driver.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/crypto.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index ed32202fa2..51f37bb1f6 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -548,6 +548,87 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)

[...]

> +            if (ret & BDRV_BLOCK_ZERO) {
> +                /* Skip zero regions */
> +            } else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
> +                       (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) {
> +                /* Count clusters we've seen */
> +                required += pnum;
> +            }

Don’t LUKS-encrypted files allocate effectively everything because zero
data has to be encrypted, too?

(“Effectively”, because you could zero out regions that are zero when
encrypted, but...)

> +        }
> +    }
> +
> +    /* Take into account preallocation.  Nothing special is needed for
> +     * PREALLOC_MODE_METADATA since metadata is always counted.
> +     */
> +    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
> +        required = virtual_size;

Same here.  I think required should always be set to virtual_size.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] qemu-img: allow qemu-img measure --object without a filename

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 512 bytes --]

On 09.01.20 12:10, Stefan Hajnoczi wrote:
> In most qemu-img sub-commands the --object option only makes sense when
> there is a filename.  qemu-img measure is an exception because objects
> may be referenced from the image creation options instead of an existing
> image file.  Allow --object without a filename.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-img.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] iotests: add 282 luks qemu-img measure test

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1530 bytes --]

On 09.01.20 12:10, Stefan Hajnoczi wrote:
> This test exercises the block/crypto.c "luks" block driver
> .bdrv_measure() code.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/282     | 93 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/282.out | 30 ++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 124 insertions(+)
>  create mode 100755 tests/qemu-iotests/282
>  create mode 100644 tests/qemu-iotests/282.out

[...]

> diff --git a/tests/qemu-iotests/282.out b/tests/qemu-iotests/282.out
> new file mode 100644
> index 0000000000..8f4737aef7
> --- /dev/null
> +++ b/tests/qemu-iotests/282.out
> @@ -0,0 +1,30 @@
> +QA output created by 282
> +== measure 1G image file ==
> +
> +required size: 2068480
> +fully allocated size: 1075810304
> +
> +== create 1G image file (size should be no greater than measured) ==
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
> +image file size in bytes: 1075810304

It clearly is greater than measured, though.  (Because zero data
generally isn’t encrypted to be zero.)

I also wonder whether it even makes much sense to check the file length,
because I suppose we’re actually interested in how much is allocated.
That is, I think it’s fine to have the file length be greater than
what’s been measured, but the allocated file size shouldn’t be.

But then again the allocated file size may well be greater because of
filesystem shenanigans...

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] luks: extract block_crypto_calculate_payload_offset()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]

On Tue, Jan 14, 2020 at 04:25:44PM +0100, Max Reitz wrote:
> On 09.01.20 12:10, Stefan Hajnoczi wrote:
> > The qcow2 .bdrv_measure() code calculates the crypto payload offset.
> > This logic really belongs in block/crypto.c where it can be reused by
> > other image formats.
> > 
> > The "luks" block driver will need this same logic in order to implement
> > .bdrv_measure(), so extract the block_crypto_calculate_payload_offset()
> > function now.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/crypto.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  block/crypto.h |  5 ++++
> >  block/qcow2.c  | 59 ++++------------------------------------------
> >  3 files changed, 73 insertions(+), 55 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 24823835c1..ed32202fa2 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -185,6 +185,70 @@ block_crypto_create_opts_init(QDict *opts, Error **errp)
> 
> [...]
> 
> > +/* Determine the number of bytes for the crypto header */
> > +bool block_crypto_calculate_payload_offset(QemuOpts *opts,
> > +                                           const char *optprefix,
> > +                                           size_t *len,
> > +                                           Error **errp)
> > +{
> > +    QDict *cryptoopts_qdict;
> > +    QCryptoBlockCreateOptions *cryptoopts;
> > +    QCryptoBlock *crypto;
> > +
> > +    /* Extract options into a qdict */
> > +    if (optprefix) {
> > +        QDict *opts_qdict = qemu_opts_to_qdict(opts, NULL);
> > +
> > +        qdict_extract_subqdict(opts_qdict, &cryptoopts_qdict, optprefix);
> > +        qobject_unref(opts_qdict);
> > +    } else {
> > +        cryptoopts_qdict = qemu_opts_to_qdict(opts, NULL);
> > +    }
> > +
> > +    /* Build QCryptoBlockCreateOptions object from qdict */
> > +    qdict_put_str(cryptoopts_qdict, "format", "luks");
> 
> Should this be a parameter?

Maybe one day, but there are no users who need it yet.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] luks: implement .bdrv_measure()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Tue, Jan 14, 2020 at 04:43:36PM +0100, Max Reitz wrote:
> On 09.01.20 12:10, Stefan Hajnoczi wrote:
> > Add qemu-img measure support in the "luks" block driver.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/crypto.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index ed32202fa2..51f37bb1f6 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -548,6 +548,87 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
> 
> [...]
> 
> > +            if (ret & BDRV_BLOCK_ZERO) {
> > +                /* Skip zero regions */
> > +            } else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
> > +                       (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) {
> > +                /* Count clusters we've seen */
> > +                required += pnum;
> > +            }
> 
> Don’t LUKS-encrypted files allocate effectively everything because zero
> data has to be encrypted, too?
> 
> (“Effectively”, because you could zero out regions that are zero when
> encrypted, but...)
> 
> > +        }
> > +    }
> > +
> > +    /* Take into account preallocation.  Nothing special is needed for
> > +     * PREALLOC_MODE_METADATA since metadata is always counted.
> > +     */
> > +    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
> > +        required = virtual_size;
> 
> Same here.  I think required should always be set to virtual_size.

Good points.  I may have inherited this from the qcow2 code, where the
L2 tables can still have unallocated/zero clusters.

I'll check if this logic makes sense outside of qcow2.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] luks: extract block_crypto_calculate_payload_offset()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2530 bytes --]

On 15.01.20 14:40, Stefan Hajnoczi wrote:
> On Tue, Jan 14, 2020 at 04:25:44PM +0100, Max Reitz wrote:
>> On 09.01.20 12:10, Stefan Hajnoczi wrote:
>>> The qcow2 .bdrv_measure() code calculates the crypto payload offset.
>>> This logic really belongs in block/crypto.c where it can be reused by
>>> other image formats.
>>>
>>> The "luks" block driver will need this same logic in order to implement
>>> .bdrv_measure(), so extract the block_crypto_calculate_payload_offset()
>>> function now.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  block/crypto.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  block/crypto.h |  5 ++++
>>>  block/qcow2.c  | 59 ++++------------------------------------------
>>>  3 files changed, 73 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/block/crypto.c b/block/crypto.c
>>> index 24823835c1..ed32202fa2 100644
>>> --- a/block/crypto.c
>>> +++ b/block/crypto.c
>>> @@ -185,6 +185,70 @@ block_crypto_create_opts_init(QDict *opts, Error **errp)
>>
>> [...]
>>
>>> +/* Determine the number of bytes for the crypto header */
>>> +bool block_crypto_calculate_payload_offset(QemuOpts *opts,
>>> +                                           const char *optprefix,
>>> +                                           size_t *len,
>>> +                                           Error **errp)
>>> +{
>>> +    QDict *cryptoopts_qdict;
>>> +    QCryptoBlockCreateOptions *cryptoopts;
>>> +    QCryptoBlock *crypto;
>>> +
>>> +    /* Extract options into a qdict */
>>> +    if (optprefix) {
>>> +        QDict *opts_qdict = qemu_opts_to_qdict(opts, NULL);
>>> +
>>> +        qdict_extract_subqdict(opts_qdict, &cryptoopts_qdict, optprefix);
>>> +        qobject_unref(opts_qdict);
>>> +    } else {
>>> +        cryptoopts_qdict = qemu_opts_to_qdict(opts, NULL);
>>> +    }
>>> +
>>> +    /* Build QCryptoBlockCreateOptions object from qdict */
>>> +    qdict_put_str(cryptoopts_qdict, "format", "luks");
>>
>> Should this be a parameter?
> 
> Maybe one day, but there are no users who need it yet.

Sure, but would it hurt? O:-)

I’m just asking because this file doesn’t implement luks crypto, so it
seems a bit strange to reference it here.  Actually, now that I think
about it...  This file only implements the luks block driver.  Is this
even the right place for the common
block_crypto_calculate_payload_offset() function?  Would it make more
sense in crypto/block.c or crypto/block-luks.c?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]