All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots
@ 2020-04-23 22:17 Eric Blake
  2020-04-23 22:17 ` [PATCH v2 1/3] block: Add blk_new_with_bs() helper Eric Blake
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Eric Blake @ 2020-04-23 22:17 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-block, mreitz

In v2:
- new patch 1 [Max]
- split off and reword unrelated change into patch 3 [Max]
- improve the test: grep for items of interest, check $? [Max]
- improve commit message explaining partial failure [Max]

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                 |  8 +++----
 block/parallels.c              |  7 +++---
 block/qcow.c                   |  7 +++---
 block/qcow2-snapshot.c         | 20 ++++++++++++----
 block/qcow2.c                  | 42 +++++++++++++++++++++++-----------
 block/qed.c                    |  7 +++---
 block/sheepdog.c               |  9 ++++----
 block/vdi.c                    |  7 +++---
 block/vhdx.c                   |  7 +++---
 block/vmdk.c                   |  9 ++++----
 block/vpc.c                    |  7 +++---
 blockdev.c                     |  8 +++----
 blockjob.c                     |  7 ++----
 tests/qemu-iotests/061         | 35 ++++++++++++++++++++++++++++
 tests/qemu-iotests/061.out     | 28 +++++++++++++++++++++++
 17 files changed, 167 insertions(+), 66 deletions(-)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] block: Add blk_new_with_bs() helper
  2020-04-23 22:17 [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots Eric Blake
@ 2020-04-23 22:17 ` Eric Blake
  2020-04-24  9:53   ` Max Reitz
  2020-04-24 12:23   ` Stefan Hajnoczi
  2020-04-23 22:17 ` [PATCH v2 2/3] qcow2: Allow resize of images with internal snapshots Eric Blake
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 15+ messages in thread
From: Eric Blake @ 2020-04-23 22:17 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, Fam Zheng, open list:Sheepdog, qemu-block, Jeff Cody,
	Stefan Weil, Markus Armbruster, mreitz, Stefan Hajnoczi,
	Liu Yuan, Denis V. Lunev, John Snow

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                 |  8 +++-----
 block/parallels.c              |  7 +++----
 block/qcow.c                   |  7 +++----
 block/qcow2.c                  | 15 ++++++---------
 block/qed.c                    |  7 +++----
 block/sheepdog.c               |  9 ++++-----
 block/vdi.c                    |  7 +++----
 block/vhdx.c                   |  7 +++----
 block/vmdk.c                   |  9 ++++-----
 block/vpc.c                    |  7 +++----
 blockdev.c                     |  8 +++-----
 blockjob.c                     |  7 ++-----
 14 files changed, 65 insertions(+), 58 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..1e082ce2a0f4 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 a block driver node 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..1cb8ae17ffde 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -261,11 +261,9 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
     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..4019557ceee3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -559,10 +559,9 @@ 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) {
         goto out;
     }
     blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/qcow.c b/block/qcow.c
index 8973e4e565a1..d9f26c515a49 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -849,10 +849,9 @@ 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) {
         goto exit;
     }
     blk_set_allow_write_beyond_eof(qcow_blk, true);
diff --git a/block/qcow2.c b/block/qcow2.c
index b524b0c53f84..1ce72041978b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3404,10 +3404,9 @@ 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) {
         goto out;
     }
     blk_set_allow_write_beyond_eof(blk, true);
@@ -5359,11 +5358,9 @@ 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);
+        BlockBackend *blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL,
+                                            errp);
+        if (!blk) {
             return ret;
         }

diff --git a/block/qed.c b/block/qed.c
index 1af9b3cb1db1..dd767a5c75c9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -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..09d75ff78dd8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1803,12 +1803,11 @@ static int sd_prealloc(BlockDriverState *bs, int64_t old_size, int64_t new_size,
     void *buf = NULL;
     int ret;

-    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..d1b692741620 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -804,10 +804,9 @@ 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) {
         goto exit;
     }

diff --git a/block/vhdx.c b/block/vhdx.c
index 33e57cd6567a..4c908f226f48 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -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..250bb0bdad78 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1012,10 +1012,9 @@ 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) {
         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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] qcow2: Allow resize of images with internal snapshots
  2020-04-23 22:17 [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots Eric Blake
  2020-04-23 22:17 ` [PATCH v2 1/3] block: Add blk_new_with_bs() helper Eric Blake
@ 2020-04-23 22:17 ` Eric Blake
  2020-04-24 10:40   ` Max Reitz
  2020-04-23 22:17 ` [PATCH v2 3/3] qcow2: Tweak comment about bitmaps vs. resize Eric Blake
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2020-04-23 22:17 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-block, mreitz

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>
---
 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 1ce72041978b..34888a793354 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3987,9 +3987,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;
     }
@@ -4951,6 +4954,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);
@@ -4968,6 +4972,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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] qcow2: Tweak comment about bitmaps vs. resize
  2020-04-23 22:17 [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots Eric Blake
  2020-04-23 22:17 ` [PATCH v2 1/3] block: Add blk_new_with_bs() helper Eric Blake
  2020-04-23 22:17 ` [PATCH v2 2/3] qcow2: Allow resize of images with internal snapshots Eric Blake
@ 2020-04-23 22:17 ` Eric Blake
  2020-04-24 10:41   ` Max Reitz
  2020-04-23 22:28 ` [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots no-reply
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2020-04-23 22:17 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-block, mreitz

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>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 34888a793354..6b6d1c3fa8b9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3997,7 +3997,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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots
  2020-04-23 22:17 [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots Eric Blake
                   ` (2 preceding siblings ...)
  2020-04-23 22:17 ` [PATCH v2 3/3] qcow2: Tweak comment about bitmaps vs. resize Eric Blake
@ 2020-04-23 22:28 ` no-reply
  2020-04-23 22:32 ` no-reply
  2020-04-23 22:35 ` no-reply
  5 siblings, 0 replies; 15+ messages in thread
From: no-reply @ 2020-04-23 22:28 UTC (permalink / raw)
  To: eblake; +Cc: kwolf, qemu-devel, qemu-block, mreitz

Patchew URL: https://patchew.org/QEMU/20200423221707.477404-1-eblake@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/qcow2-cluster.o
  CC      block/qcow2-snapshot.o
  CC      block/qcow2-cache.o
/tmp/qemu-test/src/block/qcow.c:854:9: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
    if (!qcow_blk) {
        ^~~~~~~~~
/tmp/qemu-test/src/block/qcow.c:933:12: note: uninitialized use occurs here
---
           ^
            = 0
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow.o] Error 1
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/block/qcow2.c:3409:9: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
    if (!blk) {
        ^~~~
/tmp/qemu-test/src/block/qcow2.c:3574:12: note: uninitialized use occurs here
---
           ^
            = 0
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow2.o] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=dd087ef7353844bcb604dfa47c6b0963', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-_0udd3nv/src/docker-src.2020-04-23-18.24.05.17686:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=dd087ef7353844bcb604dfa47c6b0963
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_0udd3nv/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    3m59.908s
user    0m8.451s


The full log is available at
http://patchew.org/logs/20200423221707.477404-1-eblake@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots
  2020-04-23 22:17 [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots Eric Blake
                   ` (3 preceding siblings ...)
  2020-04-23 22:28 ` [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots no-reply
@ 2020-04-23 22:32 ` no-reply
  2020-04-23 22:35 ` no-reply
  5 siblings, 0 replies; 15+ messages in thread
From: no-reply @ 2020-04-23 22:32 UTC (permalink / raw)
  To: eblake; +Cc: kwolf, qemu-devel, qemu-block, mreitz

Patchew URL: https://patchew.org/QEMU/20200423221707.477404-1-eblake@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/qed-check.o
  CC      block/vhdx.o
/tmp/qemu-test/src/block/qcow.c: In function 'qcow_co_create':
/tmp/qemu-test/src/block/qcow.c:824:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret;
         ^
cc1: all warnings being treated as errors
make: *** [block/qcow.o] Error 1
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_create':
/tmp/qemu-test/src/block/qcow2.c:3288:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret;
         ^
cc1: all warnings being treated as errors
make: *** [block/qcow2.o] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=712c8b139ba4499b8f42495e5e5ec257', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-e7ishnmm/src/docker-src.2020-04-23-18.29.54.23207:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=712c8b139ba4499b8f42495e5e5ec257
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-e7ishnmm/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m8.365s
user    0m8.548s


The full log is available at
http://patchew.org/logs/20200423221707.477404-1-eblake@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots
  2020-04-23 22:17 [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots Eric Blake
                   ` (4 preceding siblings ...)
  2020-04-23 22:32 ` no-reply
@ 2020-04-23 22:35 ` no-reply
  5 siblings, 0 replies; 15+ messages in thread
From: no-reply @ 2020-04-23 22:35 UTC (permalink / raw)
  To: eblake; +Cc: kwolf, qemu-devel, qemu-block, mreitz

Patchew URL: https://patchew.org/QEMU/20200423221707.477404-1-eblake@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/blklogwrites.o
  CC      block/block-backend.o
/tmp/qemu-test/src/block/qcow.c: In function 'qcow_co_create':
/tmp/qemu-test/src/block/qcow.c:933:12: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return ret;
            ^~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow.o] Error 1
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_create':
/tmp/qemu-test/src/block/qcow2.c:3574:12: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return ret;
            ^~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow2.o] Error 1
/tmp/qemu-test/src/block/parallels.c: In function 'parallels_co_create':
/tmp/qemu-test/src/block/parallels.c:604:12: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return ret;
            ^~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/parallels.o] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=954b57f8849a4ba29a2929c47e5b334c', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-6j971jca/src/docker-src.2020-04-23-18.32.52.29764:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=954b57f8849a4ba29a2929c47e5b334c
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-6j971jca/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m18.893s
user    0m8.916s


The full log is available at
http://patchew.org/logs/20200423221707.477404-1-eblake@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] block: Add blk_new_with_bs() helper
  2020-04-23 22:17 ` [PATCH v2 1/3] block: Add blk_new_with_bs() helper Eric Blake
@ 2020-04-24  9:53   ` Max Reitz
  2020-04-24  9:56     ` Max Reitz
  2020-04-24 12:23   ` Stefan Hajnoczi
  1 sibling, 1 reply; 15+ messages in thread
From: Max Reitz @ 2020-04-24  9:53 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, Fam Zheng, open list:Sheepdog, qemu-block, Jeff Cody,
	Stefan Weil, Markus Armbruster, Stefan Hajnoczi, Liu Yuan,
	Denis V. Lunev, John Snow


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

On 24.04.20 00:17, 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                 |  8 +++-----
>  block/parallels.c              |  7 +++----
>  block/qcow.c                   |  7 +++----
>  block/qcow2.c                  | 15 ++++++---------
>  block/qed.c                    |  7 +++----
>  block/sheepdog.c               |  9 ++++-----
>  block/vdi.c                    |  7 +++----
>  block/vhdx.c                   |  7 +++----
>  block/vmdk.c                   |  9 ++++-----
>  block/vpc.c                    |  7 +++----
>  blockdev.c                     |  8 +++-----
>  blockjob.c                     |  7 ++-----
>  14 files changed, 65 insertions(+), 58 deletions(-)

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

> diff --git a/blockdev.c b/blockdev.c
> index 5faddaa7052f..f43426ed5fbc 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -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);

This is also addressed by
https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03656.html,
but it does make sense to fix it here, too.  Well, whichever lands first
lands first, I suppose (so @ret can be dropped).

Max


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] block: Add blk_new_with_bs() helper
  2020-04-24  9:53   ` Max Reitz
@ 2020-04-24  9:56     ` Max Reitz
  2020-04-24 10:02       ` Max Reitz
  0 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2020-04-24  9:56 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, Fam Zheng, open list:Sheepdog, qemu-block, Jeff Cody,
	Stefan Weil, Markus Armbruster, Stefan Hajnoczi, Liu Yuan,
	Denis V. Lunev, John Snow


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

On 24.04.20 11:53, Max Reitz wrote:
> On 24.04.20 00:17, 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                 |  8 +++-----
>>  block/parallels.c              |  7 +++----
>>  block/qcow.c                   |  7 +++----
>>  block/qcow2.c                  | 15 ++++++---------
>>  block/qed.c                    |  7 +++----
>>  block/sheepdog.c               |  9 ++++-----
>>  block/vdi.c                    |  7 +++----
>>  block/vhdx.c                   |  7 +++----
>>  block/vmdk.c                   |  9 ++++-----
>>  block/vpc.c                    |  7 +++----
>>  blockdev.c                     |  8 +++-----
>>  blockjob.c                     |  7 ++-----
>>  14 files changed, 65 insertions(+), 58 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

(With the Patchew warning fixed, of course (i.e., we should set ret to
-EPERM or something in qcow.c))

>> diff --git a/blockdev.c b/blockdev.c
>> index 5faddaa7052f..f43426ed5fbc 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
> 
> [...]
> 
>> @@ -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);
> 
> This is also addressed by
> https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03656.html,
> but it does make sense to fix it here, too.  Well, whichever lands first
> lands first, I suppose (so @ret can be dropped).

I intended to affix the “(so @ret can be dropped)” to the first
sentence, not the second one...

Max


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] block: Add blk_new_with_bs() helper
  2020-04-24  9:56     ` Max Reitz
@ 2020-04-24 10:02       ` Max Reitz
  2020-04-24 14:18         ` Eric Blake
  0 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2020-04-24 10:02 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, Fam Zheng, open list:Sheepdog, qemu-block, Jeff Cody,
	Stefan Weil, Markus Armbruster, Stefan Hajnoczi, Liu Yuan,
	Denis V. Lunev, John Snow


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

On 24.04.20 11:56, Max Reitz wrote:
> On 24.04.20 11:53, Max Reitz wrote:
>> On 24.04.20 00:17, 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                 |  8 +++-----
>>>  block/parallels.c              |  7 +++----
>>>  block/qcow.c                   |  7 +++----
>>>  block/qcow2.c                  | 15 ++++++---------
>>>  block/qed.c                    |  7 +++----
>>>  block/sheepdog.c               |  9 ++++-----
>>>  block/vdi.c                    |  7 +++----
>>>  block/vhdx.c                   |  7 +++----
>>>  block/vmdk.c                   |  9 ++++-----
>>>  block/vpc.c                    |  7 +++----
>>>  blockdev.c                     |  8 +++-----
>>>  blockjob.c                     |  7 ++-----
>>>  14 files changed, 65 insertions(+), 58 deletions(-)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> (With the Patchew warning fixed, of course (i.e., we should set ret to
> -EPERM or something in qcow.c))

Er, well, maybe I should have looked into more places.  The compiler
only warns about that single one because it’s the only place where @ret
is really uninitialized, but there are many more where we need to set
it: crypto.c, parallels.c, qcow.c, qcow2.c (both hunks), qed.c,
sheepdog.c, vdi.c, vhdx.c, and vpc.c.

(So basically everywhere but vmdk.c, blockdev.c, and blockjob.c.)

And now I’m going to get another coffee...

Max


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/3] qcow2: Allow resize of images with internal snapshots
  2020-04-23 22:17 ` [PATCH v2 2/3] qcow2: Allow resize of images with internal snapshots Eric Blake
@ 2020-04-24 10:40   ` Max Reitz
  0 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2020-04-24 10:40 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block


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

On 24.04.20 00:17, Eric Blake wrote:
> 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>
> ---
>  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(-)

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


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] qcow2: Tweak comment about bitmaps vs. resize
  2020-04-23 22:17 ` [PATCH v2 3/3] qcow2: Tweak comment about bitmaps vs. resize Eric Blake
@ 2020-04-24 10:41   ` Max Reitz
  0 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2020-04-24 10:41 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block


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

On 24.04.20 00:17, Eric Blake wrote:
> 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>
> ---
>  block/qcow2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 34888a793354..6b6d1c3fa8b9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3997,7 +3997,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. */

Clever. ;)

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

>      if (qcow2_truncate_bitmaps_check(bs, errp)) {
>          ret = -ENOTSUP;
>          goto fail;
> 



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] block: Add blk_new_with_bs() helper
  2020-04-23 22:17 ` [PATCH v2 1/3] block: Add blk_new_with_bs() helper Eric Blake
  2020-04-24  9:53   ` Max Reitz
@ 2020-04-24 12:23   ` Stefan Hajnoczi
  1 sibling, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-04-24 12:23 UTC (permalink / raw)
  To: Eric Blake
  Cc: kwolf, Fam Zheng, open list:Sheepdog, qemu-block, Jeff Cody,
	Stefan Weil, qemu-devel, mreitz, Liu Yuan, Denis V. Lunev,
	John Snow, Markus Armbruster

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

On Thu, Apr 23, 2020 at 05:17:05PM -0500, 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                 |  8 +++-----
>  block/parallels.c              |  7 +++----
>  block/qcow.c                   |  7 +++----
>  block/qcow2.c                  | 15 ++++++---------
>  block/qed.c                    |  7 +++----
>  block/sheepdog.c               |  9 ++++-----
>  block/vdi.c                    |  7 +++----
>  block/vhdx.c                   |  7 +++----
>  block/vmdk.c                   |  9 ++++-----
>  block/vpc.c                    |  7 +++----
>  blockdev.c                     |  8 +++-----
>  blockjob.c                     |  7 ++-----
>  14 files changed, 65 insertions(+), 58 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] block: Add blk_new_with_bs() helper
  2020-04-24 10:02       ` Max Reitz
@ 2020-04-24 14:18         ` Eric Blake
  2020-04-24 14:25           ` Max Reitz
  0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2020-04-24 14:18 UTC (permalink / raw)
  To: Max Reitz, qemu-devel
  Cc: kwolf, Fam Zheng, open list:Sheepdog, qemu-block, Jeff Cody,
	Stefan Weil, Markus Armbruster, Stefan Hajnoczi, Liu Yuan,
	Denis V. Lunev, John Snow

On 4/24/20 5:02 AM, Max Reitz wrote:

>> (With the Patchew warning fixed, of course (i.e., we should set ret to
>> -EPERM or something in qcow.c))
> 
> Er, well, maybe I should have looked into more places.  The compiler
> only warns about that single one because it’s the only place where @ret
> is really uninitialized, but there are many more where we need to set
> it: crypto.c, parallels.c, qcow.c, qcow2.c (both hunks), qed.c,
> sheepdog.c, vdi.c, vhdx.c, and vpc.c.
> 
> (So basically everywhere but vmdk.c, blockdev.c, and blockjob.c.)

Urgh - so it's even less of a win in terms of reduced lines of code. 
Still, I'll fix it and post v3.

> 
> And now I’m going to get another coffee...
> 
> Max
> 

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] block: Add blk_new_with_bs() helper
  2020-04-24 14:18         ` Eric Blake
@ 2020-04-24 14:25           ` Max Reitz
  0 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2020-04-24 14:25 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, Fam Zheng, open list:Sheepdog, qemu-block, Jeff Cody,
	Stefan Weil, Markus Armbruster, Stefan Hajnoczi, Liu Yuan,
	Denis V. Lunev, John Snow


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

On 24.04.20 16:18, Eric Blake wrote:
> On 4/24/20 5:02 AM, Max Reitz wrote:
> 
>>> (With the Patchew warning fixed, of course (i.e., we should set ret to
>>> -EPERM or something in qcow.c))
>>
>> Er, well, maybe I should have looked into more places.  The compiler
>> only warns about that single one because it’s the only place where @ret
>> is really uninitialized, but there are many more where we need to set
>> it: crypto.c, parallels.c, qcow.c, qcow2.c (both hunks), qed.c,
>> sheepdog.c, vdi.c, vhdx.c, and vpc.c.
>>
>> (So basically everywhere but vmdk.c, blockdev.c, and blockjob.c.)
> 
> Urgh - so it's even less of a win in terms of reduced lines of code.

True, but I still consider it a win in terms of reduced complexity.
Before, we have two function calls with two variable assignments from
their return values (@blk and @ret).  Afterwards, we’ll have a single
function all with one variable assignment from its return value (@blk),
and one stand-alone variable assignment with a constant value (“ret =
-EPERM” or similar). :)

Max


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-04-24 14:26 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 22:17 [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots Eric Blake
2020-04-23 22:17 ` [PATCH v2 1/3] block: Add blk_new_with_bs() helper Eric Blake
2020-04-24  9:53   ` Max Reitz
2020-04-24  9:56     ` Max Reitz
2020-04-24 10:02       ` Max Reitz
2020-04-24 14:18         ` Eric Blake
2020-04-24 14:25           ` Max Reitz
2020-04-24 12:23   ` Stefan Hajnoczi
2020-04-23 22:17 ` [PATCH v2 2/3] qcow2: Allow resize of images with internal snapshots Eric Blake
2020-04-24 10:40   ` Max Reitz
2020-04-23 22:17 ` [PATCH v2 3/3] qcow2: Tweak comment about bitmaps vs. resize Eric Blake
2020-04-24 10:41   ` Max Reitz
2020-04-23 22:28 ` [PATCH v2 0/3] qcow2: Allow resize of images with internal snapshots no-reply
2020-04-23 22:32 ` no-reply
2020-04-23 22:35 ` no-reply

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.