[PATCH v3 0/3] qcow2: Allow resize of images with internal snapshots

[PATCH v3 0/3] qcow2: Allow resize of images with internal snapshots

[Eric Blake]

In v3:
- patch 1: fix error returns [patchew, Max], R-b dropped
- patch 2,3: unchanged, so add R-b

Eric Blake (3):
  block: Add blk_new_with_bs() helper
  qcow2: Allow resize of images with internal snapshots
  qcow2: Tweak comment about bitmaps vs. resize

 include/sysemu/block-backend.h |  2 ++
 block/block-backend.c          | 23 +++++++++++++++++
 block/crypto.c                 | 10 +++-----
 block/parallels.c              |  8 +++---
 block/qcow.c                   |  8 +++---
 block/qcow2-snapshot.c         | 20 ++++++++++++---
 block/qcow2.c                  | 45 +++++++++++++++++++++++-----------
 block/qed.c                    |  9 +++----
 block/sheepdog.c               | 11 ++++-----
 block/vdi.c                    |  8 +++---
 block/vhdx.c                   |  9 +++----
 block/vmdk.c                   |  9 +++----
 block/vpc.c                    |  8 +++---
 blockdev.c                     |  8 +++---
 blockjob.c                     |  7 ++----
 tests/qemu-iotests/061         | 35 ++++++++++++++++++++++++++
 tests/qemu-iotests/061.out     | 28 +++++++++++++++++++++
 17 files changed, 177 insertions(+), 71 deletions(-)

-- 
2.26.2

[PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Eric Blake]

There are several callers that need to create a new block backend from
an existing BDS; make the task slightly easier with a common helper
routine.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/sysemu/block-backend.h |  2 ++
 block/block-backend.c          | 23 +++++++++++++++++++++++
 block/crypto.c                 | 10 ++++------
 block/parallels.c              |  8 ++++----
 block/qcow.c                   |  8 ++++----
 block/qcow2.c                  | 18 ++++++++----------
 block/qed.c                    |  9 ++++-----
 block/sheepdog.c               | 11 +++++------
 block/vdi.c                    |  8 ++++----
 block/vhdx.c                   |  9 ++++-----
 block/vmdk.c                   |  9 ++++-----
 block/vpc.c                    |  8 ++++----
 blockdev.c                     |  8 +++-----
 blockjob.c                     |  7 ++-----
 14 files changed, 75 insertions(+), 63 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9bbdbd63d743..d37c1244dd9c 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -77,6 +77,8 @@ typedef struct BlockBackendPublic {
 } BlockBackendPublic;

 BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm);
+BlockBackend *blk_new_with_bs(BlockDriverState *bs, uint64_t perm,
+                              uint64_t shared_perm, Error **errp);
 BlockBackend *blk_new_open(const char *filename, const char *reference,
                            QDict *options, int flags, Error **errp);
 int blk_get_refcnt(BlockBackend *blk);
diff --git a/block/block-backend.c b/block/block-backend.c
index 38ae41382652..3592066b4259 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -355,6 +355,29 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)
     return blk;
 }

+/*
+ * Create a new BlockBackend connected to an existing BlockDriverState.
+ *
+ * @perm is a bitmasks of BLK_PERM_* constants which describes the
+ * permissions to request for @bs that is attached to this
+ * BlockBackend.  @shared_perm is a bitmask which describes which
+ * permissions may be granted to other users of the attached node.
+ * Both sets of permissions can be changed later using blk_set_perm().
+ *
+ * Return the new BlockBackend on success, null on failure.
+ */
+BlockBackend *blk_new_with_bs(BlockDriverState *bs, uint64_t perm,
+                              uint64_t shared_perm, Error **errp)
+{
+    BlockBackend *blk = blk_new(bdrv_get_aio_context(bs), perm, shared_perm);
+
+    if (blk_insert_bs(blk, bs, errp) < 0) {
+        blk_unref(blk);
+        return NULL;
+    }
+    return blk;
+}
+
 /*
  * Creates a new BlockBackend, opens a new BlockDriverState, and connects both.
  * The new BlockBackend is in the main AioContext.
diff --git a/block/crypto.c b/block/crypto.c
index d577f89659fa..a4d77f07fe8c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -256,16 +256,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
                                           PreallocMode prealloc,
                                           Error **errp)
 {
-    int ret;
+    int ret = -EPERM;
     BlockBackend *blk;
     QCryptoBlock *crypto = NULL;
     struct BlockCryptoCreateData data;

-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                          errp);
+    if (!blk) {
         goto cleanup;
     }

diff --git a/block/parallels.c b/block/parallels.c
index 6d4ed77f165f..aad07b89c818 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -559,10 +559,10 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
         return -EIO;
     }

-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                          errp);
+    if (!blk) {
+        ret = -EPERM;
         goto out;
     }
     blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/qcow.c b/block/qcow.c
index 8973e4e565a1..3f0c60051933 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -849,10 +849,10 @@ static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
         return -EIO;
     }

-    qcow_blk = blk_new(bdrv_get_aio_context(bs),
-                       BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(qcow_blk, bs, errp);
-    if (ret < 0) {
+    qcow_blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
+                               BLK_PERM_ALL, errp);
+    if (!qcow_blk) {
+        ret = -EPERM;
         goto exit;
     }
     blk_set_allow_write_beyond_eof(qcow_blk, true);
diff --git a/block/qcow2.c b/block/qcow2.c
index b524b0c53f84..f56745c56924 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3404,10 +3404,10 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
     }

     /* Create BlockBackend to write to the image */
-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                          errp);
+    if (!blk) {
+        ret = -EPERM;
         goto out;
     }
     blk_set_allow_write_beyond_eof(blk, true);
@@ -5359,12 +5359,10 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
     }

     if (new_size) {
-        BlockBackend *blk = blk_new(bdrv_get_aio_context(bs),
-                                    BLK_PERM_RESIZE, BLK_PERM_ALL);
-        ret = blk_insert_bs(blk, bs, errp);
-        if (ret < 0) {
-            blk_unref(blk);
-            return ret;
+        BlockBackend *blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL,
+                                            errp);
+        if (!blk) {
+            return -EPERM;
         }

         /*
diff --git a/block/qed.c b/block/qed.c
index 1af9b3cb1db1..7a31495d293b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -610,7 +610,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
     QEDHeader le_header;
     uint8_t *l1_table = NULL;
     size_t l1_size;
-    int ret = 0;
+    int ret = -EPERM;

     assert(opts->driver == BLOCKDEV_DRIVER_QED);
     qed_opts = &opts->u.qed;
@@ -651,10 +651,9 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
         return -EIO;
     }

-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                          errp);
+    if (!blk) {
         goto out;
     }
     blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 59f7ebb1710f..2b53cd950d20 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1801,14 +1801,13 @@ static int sd_prealloc(BlockDriverState *bs, int64_t old_size, int64_t new_size,
     uint32_t idx, max_idx;
     uint32_t object_size;
     void *buf = NULL;
-    int ret;
+    int ret = -EPERM;

-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
-                  BLK_PERM_ALL);
+    blk = blk_new_with_bs(bs,
+                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
+                          BLK_PERM_ALL, errp);

-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    if (!blk) {
         goto out_with_err_set;
     }

diff --git a/block/vdi.c b/block/vdi.c
index e1a11f2aa097..0e90130268cd 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -804,10 +804,10 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
         goto exit;
     }

-    blk = blk_new(bdrv_get_aio_context(bs_file),
-                  BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(blk, bs_file, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs_file, BLK_PERM_WRITE | BLK_PERM_RESIZE,
+                          BLK_PERM_ALL, errp);
+    if (!blk) {
+        ret = -EPERM;
         goto exit;
     }

diff --git a/block/vhdx.c b/block/vhdx.c
index 33e57cd6567a..bdf5d05cc018 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1891,7 +1891,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;

-    int ret = 0;
+    int ret = -EPERM;
     uint64_t image_size;
     uint32_t log_size;
     uint32_t block_size;
@@ -1984,10 +1984,9 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         return -EIO;
     }

-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                          errp);
+    if (!blk) {
         goto delete_and_exit;
     }
     blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/vmdk.c b/block/vmdk.c
index 218d9c980059..dff9bd28cc3f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2717,11 +2717,10 @@ static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
     if (!bs) {
         return NULL;
     }
-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
-                  BLK_PERM_ALL);
-    if (blk_insert_bs(blk, bs, errp)) {
-        bdrv_unref(bs);
+    blk = blk_new_with_bs(bs,
+                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
+                          BLK_PERM_ALL, errp);
+    if (!blk) {
         return NULL;
     }
     blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/vpc.c b/block/vpc.c
index d8141b52da8b..4858083c20e4 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1012,10 +1012,10 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
         return -EIO;
     }

-    blk = blk_new(bdrv_get_aio_context(bs),
-                  BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                          errp);
+    if (!blk) {
+        ret = -EPERM;
         goto out;
     }
     blk_set_allow_write_beyond_eof(blk, true);
diff --git a/blockdev.c b/blockdev.c
index 5faddaa7052f..f43426ed5fbc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2711,7 +2711,6 @@ void qmp_block_resize(bool has_device, const char *device,
     BlockBackend *blk = NULL;
     BlockDriverState *bs;
     AioContext *aio_context;
-    int ret;

     bs = bdrv_lookup_bs(has_device ? device : NULL,
                         has_node_name ? node_name : NULL,
@@ -2734,14 +2733,13 @@ void qmp_block_resize(bool has_device, const char *device,
         goto out;
     }

-    blk = blk_new(bdrv_get_aio_context(bs), BLK_PERM_RESIZE, BLK_PERM_ALL);
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
+    blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
+    if (!blk) {
         goto out;
     }

     bdrv_drained_begin(bs);
-    ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
+    blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
     bdrv_drained_end(bs);

 out:
diff --git a/blockjob.c b/blockjob.c
index fc850312c124..2affa1844d4f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -397,16 +397,13 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
 {
     BlockBackend *blk;
     BlockJob *job;
-    int ret;

     if (job_id == NULL && !(flags & JOB_INTERNAL)) {
         job_id = bdrv_get_device_name(bs);
     }

-    blk = blk_new(bdrv_get_aio_context(bs), perm, shared_perm);
-    ret = blk_insert_bs(blk, bs, errp);
-    if (ret < 0) {
-        blk_unref(blk);
+    blk = blk_new_with_bs(bs, perm, shared_perm, errp);
+    if (!blk) {
         return NULL;
     }

-- 
2.26.2

Re: [PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Max Reitz]

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

On 24.04.20 21:09, Eric Blake wrote:
> There are several callers that need to create a new block backend from
> an existing BDS; make the task slightly easier with a common helper
> routine.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/sysemu/block-backend.h |  2 ++
>  block/block-backend.c          | 23 +++++++++++++++++++++++
>  block/crypto.c                 | 10 ++++------
>  block/parallels.c              |  8 ++++----
>  block/qcow.c                   |  8 ++++----
>  block/qcow2.c                  | 18 ++++++++----------
>  block/qed.c                    |  9 ++++-----
>  block/sheepdog.c               | 11 +++++------
>  block/vdi.c                    |  8 ++++----
>  block/vhdx.c                   |  9 ++++-----
>  block/vmdk.c                   |  9 ++++-----
>  block/vpc.c                    |  8 ++++----
>  blockdev.c                     |  8 +++-----
>  blockjob.c                     |  7 ++-----
>  14 files changed, 75 insertions(+), 63 deletions(-)

[...]

> diff --git a/block/crypto.c b/block/crypto.c
> index d577f89659fa..a4d77f07fe8c 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -256,16 +256,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
>                                            PreallocMode prealloc,
>                                            Error **errp)
>  {
> -    int ret;
> +    int ret = -EPERM;

I’m not sure I’m a fan of this, because I feel like it makes the code
harder to read, due to having to look in three places (here, around the
blk_new_with_bs() call, and under the cleanup label) instead of in two
(not here) to verify that the error handling code is correct.

There’s also the fact that this is not really a default return value,
but one very specific error code for if one very specific function call
fails.

I suppose it comes down to whether one considers LoC a complexity
problem.  (I don’t, necessarily.)

(Also I realize it seems rather common in the kernel to set error return
variables before the function call, but I think the more common pattern
in qemu is to set it in the error path.)

[...]

> diff --git a/block/qed.c b/block/qed.c
> index 1af9b3cb1db1..7a31495d293b 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -610,7 +610,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
>      QEDHeader le_header;
>      uint8_t *l1_table = NULL;
>      size_t l1_size;
> -    int ret = 0;
> +    int ret = -EPERM;

(Same here; well, especially here, because there is so much other code
between the initialization and the “goto out” this is for)

[...]

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 59f7ebb1710f..2b53cd950d20 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1801,14 +1801,13 @@ static int sd_prealloc(BlockDriverState *bs, int64_t old_size, int64_t new_size,
>      uint32_t idx, max_idx;
>      uint32_t object_size;
>      void *buf = NULL;
> -    int ret;
> +    int ret = -EPERM;

(and here)

[...]

> diff --git a/block/vhdx.c b/block/vhdx.c
> index 33e57cd6567a..bdf5d05cc018 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1891,7 +1891,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
>      BlockBackend *blk = NULL;
>      BlockDriverState *bs = NULL;
> 
> -    int ret = 0;
> +    int ret = -EPERM;

(and again especially here)

But it does look like you got all cases covered this time.

Max


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

Re: [PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Eric Blake]

On 4/27/20 5:00 AM, Max Reitz wrote:
> On 24.04.20 21:09, Eric Blake wrote:
>> There are several callers that need to create a new block backend from
>> an existing BDS; make the task slightly easier with a common helper
>> routine.
>>
>> Suggested-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---

>> +++ b/block/crypto.c
>> @@ -256,16 +256,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
>>                                             PreallocMode prealloc,
>>                                             Error **errp)
>>   {
>> -    int ret;
>> +    int ret = -EPERM;
> 
> I’m not sure I’m a fan of this, because I feel like it makes the code
> harder to read, due to having to look in three places (here, around the
> blk_new_with_bs() call, and under the cleanup label) instead of in two
> (not here) to verify that the error handling code is correct.
> 
> There’s also the fact that this is not really a default return value,
> but one very specific error code for if one very specific function call
> fails.
> 
> I suppose it comes down to whether one considers LoC a complexity
> problem.  (I don’t, necessarily.)
> 
> (Also I realize it seems rather common in the kernel to set error return
> variables before the function call, but I think the more common pattern
> in qemu is to set it in the error path.)

I'm fine with either style.  Setting it up front is handy if that 
particular error makes a good default, but in many of the functions I 
touched, we were returning a variety of errors (-EIO, -EINVAL, -EPERM, 
etc) such that there was no good default, and thus no reason to set a 
default up front.  Is this something that would go through your tree, 
and if so, are you okay making that tweak, or do I need to send v4?

> 
> But it does look like you got all cases covered this time.

Good to hear.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Max Reitz]

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

On 27.04.20 16:03, Eric Blake wrote:
> On 4/27/20 5:00 AM, Max Reitz wrote:
>> On 24.04.20 21:09, Eric Blake wrote:
>>> There are several callers that need to create a new block backend from
>>> an existing BDS; make the task slightly easier with a common helper
>>> routine.
>>>
>>> Suggested-by: Max Reitz <mreitz@redhat.com>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
> 
>>> +++ b/block/crypto.c
>>> @@ -256,16 +256,14 @@ static int
>>> block_crypto_co_create_generic(BlockDriverState *bs,
>>>                                             PreallocMode prealloc,
>>>                                             Error **errp)
>>>   {
>>> -    int ret;
>>> +    int ret = -EPERM;
>>
>> I’m not sure I’m a fan of this, because I feel like it makes the code
>> harder to read, due to having to look in three places (here, around the
>> blk_new_with_bs() call, and under the cleanup label) instead of in two
>> (not here) to verify that the error handling code is correct.
>>
>> There’s also the fact that this is not really a default return value,
>> but one very specific error code for if one very specific function call
>> fails.
>>
>> I suppose it comes down to whether one considers LoC a complexity
>> problem.  (I don’t, necessarily.)
>>
>> (Also I realize it seems rather common in the kernel to set error return
>> variables before the function call, but I think the more common pattern
>> in qemu is to set it in the error path.)
> 
> I'm fine with either style.  Setting it up front is handy if that
> particular error makes a good default, but in many of the functions I
> touched, we were returning a variety of errors (-EIO, -EINVAL, -EPERM,
> etc) such that there was no good default, and thus no reason to set a
> default up front.  Is this something that would go through your tree,
> and if so, are you okay making that tweak, or do I need to send v4?

I suppose I can do that, this is what I’d squash in, OK?

Max

diff --git a/block/crypto.c b/block/crypto.c
index a4d77f07fe..d09b364134 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -256,7 +256,7 @@ static int
block_crypto_co_create_generic(BlockDriverState *bs,
                                           PreallocMode prealloc,
                                           Error **errp)
 {
-    int ret = -EPERM;
+    int ret;
     BlockBackend *blk;
     QCryptoBlock *crypto = NULL;
     struct BlockCryptoCreateData data;
@@ -264,6 +264,7 @@ static int
block_crypto_co_create_generic(BlockDriverState *bs,
     blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
BLK_PERM_ALL,
                           errp);
     if (!blk) {
+        ret = -EPERM;
         goto cleanup;
     }

diff --git a/block/qed.c b/block/qed.c
index 7a31495d29..671742511e 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -610,7 +610,7 @@ static int coroutine_fn
bdrv_qed_co_create(BlockdevCreateOptions *opts,
     QEDHeader le_header;
     uint8_t *l1_table = NULL;
     size_t l1_size;
-    int ret = -EPERM;
+    int ret = 0;

     assert(opts->driver == BLOCKDEV_DRIVER_QED);
     qed_opts = &opts->u.qed;
@@ -654,6 +654,7 @@ static int coroutine_fn
bdrv_qed_co_create(BlockdevCreateOptions *opts,
     blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
BLK_PERM_ALL,
                           errp);
     if (!blk) {
+        ret = -EPERM;
         goto out;
     }
     blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 2b53cd950d..8baf9e66bb 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1801,13 +1801,14 @@ static int sd_prealloc(BlockDriverState *bs,
int64_t old_size, int64_t new_size,
     uint32_t idx, max_idx;
     uint32_t object_size;
     void *buf = NULL;
-    int ret = -EPERM;
+    int ret;

     blk = blk_new_with_bs(bs,
                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
BLK_PERM_RESIZE,
                           BLK_PERM_ALL, errp);

     if (!blk) {
+        ret = -EPERM;
         goto out_with_err_set;
     }

diff --git a/block/vhdx.c b/block/vhdx.c
index bdf5d05cc0..8ca6976b59 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1891,7 +1891,7 @@ static int coroutine_fn
vhdx_co_create(BlockdevCreateOptions *opts,
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;

-    int ret = -EPERM;
+    int ret = 0;
     uint64_t image_size;
     uint32_t log_size;
     uint32_t block_size;
@@ -1987,6 +1987,7 @@ static int coroutine_fn
vhdx_co_create(BlockdevCreateOptions *opts,
     blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
BLK_PERM_ALL,
                           errp);
     if (!blk) {
+        ret = -EPERM;
         goto delete_and_exit;
     }
     blk_set_allow_write_beyond_eof(blk, true);


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

Re: [PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Eric Blake]

On 4/28/20 1:34 AM, Max Reitz wrote:

>>>> block_crypto_co_create_generic(BlockDriverState *bs,
>>>>                                              PreallocMode prealloc,
>>>>                                              Error **errp)
>>>>    {
>>>> -    int ret;
>>>> +    int ret = -EPERM;
>>>
>>> I’m not sure I’m a fan of this, because I feel like it makes the code
>>> harder to read, due to having to look in three places (here, around the
>>> blk_new_with_bs() call, and under the cleanup label) instead of in two
>>> (not here) to verify that the error handling code is correct.
>>>
>>> There’s also the fact that this is not really a default return value,
>>> but one very specific error code for if one very specific function call
>>> fails.
>>>
>>> I suppose it comes down to whether one considers LoC a complexity
>>> problem.  (I don’t, necessarily.)
>>>
>>> (Also I realize it seems rather common in the kernel to set error return
>>> variables before the function call, but I think the more common pattern
>>> in qemu is to set it in the error path.)
>>
>> I'm fine with either style.  Setting it up front is handy if that
>> particular error makes a good default, but in many of the functions I
>> touched, we were returning a variety of errors (-EIO, -EINVAL, -EPERM,
>> etc) such that there was no good default, and thus no reason to set a
>> default up front.  Is this something that would go through your tree,
>> and if so, are you okay making that tweak, or do I need to send v4?
> 
> I suppose I can do that, this is what I’d squash in, OK?

Yes, that change looks correct to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v3 2/3] qcow2: Allow resize of images with internal snapshots

[Eric Blake]

We originally refused to allow resize of images with internal
snapshots because the v2 image format did not require the tracking of
snapshot size, making it impossible to safely revert to a snapshot
with a different size than the current view of the image.  But the
snapshot size tracking was rectified in v3, and our recent fixes to
qemu-img amend (see 0a85af35) guarantee that we always have a valid
snapshot size.  Thus, we no longer need to artificially limit image
resizes, but it does become one more thing that would prevent a
downgrade back to v2.  And now that we support different-sized
snapshots, it's also easy to fix reverting to a snapshot to apply the
new size.

Upgrade iotest 61 to cover this (we previously had NO coverage of
refusal to resize while snapshots exist).  Note that the amend process
can fail but still have effects: in particular, since we break things
into upgrade, resize, downgrade, a failure during resize does not roll
back changes made during upgrade, nor does failure in downgrade roll
back a resize.  But this situation is pre-existing even without this
patch; and without journaling, the best we could do is minimize the
chance of partial failure by collecting all changes prior to doing any
writes - which adds a lot of complexity but could still fail with EIO.
On the other hand, we are careful that even if we have partial
modification but then fail, the image is left viable (that is, we are
careful to sequence things so that after each successful cluster
write, there may be transient leaked clusters but no corrupt
metadata).  And complicating the code to make it more transaction-like
is not worth the effort: a user can always request multiple 'qemu-img
amend' changing one thing each, if they need finer-grained control
over detecting the first failure than what they get by letting qemu
decide how to sequence multiple changes.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c     | 20 ++++++++++++++++----
 block/qcow2.c              | 25 ++++++++++++++++++++++---
 tests/qemu-iotests/061     | 35 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/061.out | 28 ++++++++++++++++++++++++++++
 4 files changed, 101 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 82c32d4c9b08..df16424fd952 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -23,6 +23,7 @@
  */

 #include "qemu/osdep.h"
+#include "sysemu/block-backend.h"
 #include "qapi/error.h"
 #include "qcow2.h"
 #include "qemu/bswap.h"
@@ -775,10 +776,21 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     }

     if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
-        error_report("qcow2: Loading snapshots with different disk "
-            "size is not implemented");
-        ret = -ENOTSUP;
-        goto fail;
+        BlockBackend *blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL,
+                                            &local_err);
+        if (!blk) {
+            error_report_err(local_err);
+            ret = -ENOTSUP;
+            goto fail;
+        }
+
+        ret = blk_truncate(blk, sn->disk_size, true, PREALLOC_MODE_OFF,
+                           &local_err);
+        blk_unref(blk);
+        if (ret < 0) {
+            error_report_err(local_err);
+            goto fail;
+        }
     }

     /*
diff --git a/block/qcow2.c b/block/qcow2.c
index f56745c56924..bf739cd06be0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3988,9 +3988,12 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,

     qemu_co_mutex_lock(&s->lock);

-    /* cannot proceed if image has snapshots */
-    if (s->nb_snapshots) {
-        error_setg(errp, "Can't resize an image which has snapshots");
+    /*
+     * Even though we store snapshot size for all images, it was not
+     * required until v3, so it is not safe to proceed for v2.
+     */
+    if (s->nb_snapshots && s->qcow_version < 3) {
+        error_setg(errp, "Can't resize a v2 image which has snapshots");
         ret = -ENOTSUP;
         goto fail;
     }
@@ -4952,6 +4955,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
     BDRVQcow2State *s = bs->opaque;
     int current_version = s->qcow_version;
     int ret;
+    int i;

     /* This is qcow2_downgrade(), not qcow2_upgrade() */
     assert(target_version < current_version);
@@ -4969,6 +4973,21 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
         return -ENOTSUP;
     }

+    /*
+     * If any internal snapshot has a different size than the current
+     * image size, or VM state size that exceeds 32 bits, downgrading
+     * is unsafe.  Even though we would still use v3-compliant output
+     * to preserve that data, other v2 programs might not realize
+     * those optional fields are important.
+     */
+    for (i = 0; i < s->nb_snapshots; i++) {
+        if (s->snapshots[i].vm_state_size > UINT32_MAX ||
+            s->snapshots[i].disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
+            error_setg(errp, "Internal snapshots prevent downgrade of image");
+            return -ENOTSUP;
+        }
+    }
+
     /* clear incompatible features */
     if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
         ret = qcow2_mark_clean(bs);
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index ce285d308408..10eb24316461 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -111,6 +111,41 @@ $PYTHON qcow2.py "$TEST_IMG" dump-header
 $QEMU_IO -c "read -P 0x2a 42M 64k" "$TEST_IMG" | _filter_qemu_io
 _check_test_img

+echo
+echo "=== Testing resize with snapshots ==="
+echo
+_make_test_img -o "compat=0.10" 32M
+$QEMU_IO -c "write -P 0x2a 24M 64k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -c foo "$TEST_IMG"
+$QEMU_IMG resize "$TEST_IMG" 64M &&
+    echo "unexpected pass"
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep '^\(version\|size\|nb_snap\)'
+
+$QEMU_IMG amend -o "compat=1.1,size=128M" "$TEST_IMG" ||
+    echo "unexpected fail"
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep '^\(version\|size\|nb_snap\)'
+
+$QEMU_IMG snapshot -c bar "$TEST_IMG"
+$QEMU_IMG resize --shrink "$TEST_IMG" 64M ||
+    echo "unexpected fail"
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep '^\(version\|size\|nb_snap\)'
+
+$QEMU_IMG amend -o "compat=0.10,size=32M" "$TEST_IMG" &&
+    echo "unexpected pass"
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep '^\(version\|size\|nb_snap\)'
+
+$QEMU_IMG snapshot -a bar "$TEST_IMG" ||
+    echo "unexpected fail"
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep '^\(version\|size\|nb_snap\)'
+
+$QEMU_IMG snapshot -d bar "$TEST_IMG"
+$QEMU_IMG amend -o "compat=0.10,size=32M" "$TEST_IMG" ||
+    echo "unexpected fail"
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep '^\(version\|size\|nb_snap\)'
+
+_check_test_img
+
+
 echo
 echo "=== Testing dirty lazy_refcounts=off ==="
 echo
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 413cc4e0f4ab..5a8d36d0058a 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -271,6 +271,34 @@ read 65536/65536 bytes at offset 44040192
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 No errors were found on the image.

+=== Testing resize with snapshots ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432
+wrote 65536/65536 bytes at offset 25165824
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: Can't resize a v2 image which has snapshots
+version                   2
+size                      33554432
+nb_snapshots              1
+version                   3
+size                      134217728
+nb_snapshots              1
+Image resized.
+version                   3
+size                      67108864
+nb_snapshots              2
+qemu-img: Internal snapshots prevent downgrade of image
+version                   3
+size                      33554432
+nb_snapshots              2
+version                   3
+size                      134217728
+nb_snapshots              2
+version                   2
+size                      33554432
+nb_snapshots              1
+No errors were found on the image.
+
 === Testing dirty lazy_refcounts=off ===

 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.26.2

[PATCH v3 3/3] qcow2: Tweak comment about bitmaps vs. resize

[Eric Blake]

Our comment did not actually match the code.  Rewrite the comment to
be less sensitive to any future changes to qcow2-bitmap.c that might
implement scenarios that we currently reject.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index bf739cd06be0..81b75401813c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3998,7 +3998,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         goto fail;
     }

-    /* cannot proceed if image has bitmaps */
+    /* See qcow2-bitmap.c for which bitmap scenarios prevent a resize. */
     if (qcow2_truncate_bitmaps_check(bs, errp)) {
         ret = -ENOTSUP;
         goto fail;
-- 
2.26.2

Re: [PATCH v3 0/3] qcow2: Allow resize of images with internal snapshots

[Max Reitz]

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

On 24.04.20 21:09, Eric Blake wrote:
> In v3:
> - patch 1: fix error returns [patchew, Max], R-b dropped
> - patch 2,3: unchanged, so add R-b
> 
> Eric Blake (3):
>   block: Add blk_new_with_bs() helper
>   qcow2: Allow resize of images with internal snapshots
>   qcow2: Tweak comment about bitmaps vs. resize

Thanks, I’ve squashed the diff into patch 1 and applied the series to my
block-next branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

Max


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

Re: [PATCH v3 0/3] qcow2: Allow resize of images with internal snapshots

[Eric Blake]

On 4/28/20 7:59 AM, Max Reitz wrote:
> On 24.04.20 21:09, Eric Blake wrote:
>> In v3:
>> - patch 1: fix error returns [patchew, Max], R-b dropped
>> - patch 2,3: unchanged, so add R-b
>>
>> Eric Blake (3):
>>    block: Add blk_new_with_bs() helper
>>    qcow2: Allow resize of images with internal snapshots
>>    qcow2: Tweak comment about bitmaps vs. resize
> 
> Thanks, I’ve squashed the diff into patch 1 and applied the series to my
> block-next branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

This series has not only a merge conflict, but a semantic conflict, with 
the current state of Kevin's block-next branch.  I think I'll go ahead 
and post a v4 based on Kevin's branch to spare you the efforts of having 
to repeat my merge resolution.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org