[Qemu-devel] [PATCH v2 0/2] backup: Fail early if cannot determine cluster size

[Qemu-devel] [PATCH v2 0/2] backup: Fail early if cannot determine cluster size

[Fam Zheng]

v2: More accurately, s:hang:trigger assertion failure: in the bug fix patch
    commit message.
    Add John's and Jeff's R-b.
    Add a test. [Eric]



Fam Zheng (2):
  backup: Fail early if cannot determine cluster size
  iotests: Add regression test for drive-backup cluster size error

 block/backup.c             | 34 ++++++++++++++++++----------------
 tests/qemu-iotests/055     | 13 +++++++++++++
 tests/qemu-iotests/055.out |  4 ++--
 3 files changed, 33 insertions(+), 18 deletions(-)

-- 
2.8.2

[Qemu-devel] [PATCH v2 1/2] backup: Fail early if cannot determine cluster size

[Fam Zheng]

Otherwise the job is orphaned and block_job_cancel_sync in
bdrv_close_all() when quitting will trigger assertion failure.

Cc: qemu-stable@nongnu.org
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 491fd14..3ff48c6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -505,6 +505,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     int64_t len;
     BlockDriverInfo bdi;
     int ret;
+    int64_t cluster_size;
 
     assert(bs);
     assert(target);
@@ -568,23 +569,10 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         goto error;
     }
 
-    BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
-                                           cb, opaque, errp);
-    if (!job) {
-        goto error;
-    }
-
-    job->on_source_error = on_source_error;
-    job->on_target_error = on_target_error;
-    job->target = target;
-    job->sync_mode = sync_mode;
-    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
-                       sync_bitmap : NULL;
-
     /* If there is no backing file on the target, we cannot rely on COW if our
      * backup cluster size is smaller than the target cluster size. Even for
      * targets with a backing file, try to avoid COW if possible. */
-    ret = bdrv_get_info(job->target, &bdi);
+    ret = bdrv_get_info(target, &bdi);
     if (ret < 0 && !target->backing) {
         error_setg_errno(errp, -ret,
             "Couldn't determine the cluster size of the target image, "
@@ -594,11 +582,25 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         goto error;
     } else if (ret < 0 && target->backing) {
         /* Not fatal; just trudge on ahead. */
-        job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
+        cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
     } else {
-        job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+        cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
     }
 
+    BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
+                                           cb, opaque, errp);
+    if (!job) {
+        goto error;
+    }
+
+    job->on_source_error = on_source_error;
+    job->on_target_error = on_target_error;
+    job->target = target;
+    job->sync_mode = sync_mode;
+    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
+                       sync_bitmap : NULL;
+    job->cluster_size = cluster_size;
+
     bdrv_op_block_all(target, job->common.blocker);
     job->common.len = len;
     job->common.co = qemu_coroutine_create(backup_run);
-- 
2.8.2

Re: [Qemu-devel] [PATCH v2 1/2] backup: Fail early if cannot determine cluster size

[Kevin Wolf]

Am 19.05.2016 um 03:25 hat Fam Zheng geschrieben:
> Otherwise the job is orphaned and block_job_cancel_sync in
> bdrv_close_all() when quitting will trigger assertion failure.
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>

Instead of moving the code around, wouldn't it have been easier to just
add the missing block_job_unref() call? That would also seem safer with
respect to future changes in the code that might reintroduce new error
conditions after block_job_create() and resurrect this bug.

That said, the patch looks correct, so if you don't want to change it:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v2 2/2] iotests: Add regression test for drive-backup cluster size error

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/055     | 13 +++++++++++++
 tests/qemu-iotests/055.out |  4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index c8e3578..7f990c2 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -451,5 +451,18 @@ class TestSingleTransaction(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_no_active_block_jobs()
 
+class TestNullToNull(iotests.QMPTestCase):
+
+    def test_backup(self):
+        """blockdev-backup from null-co:// to null-co:// shouldn't leak job"""
+        self.vm = iotests.VM().\
+                add_drive_raw("id=drive0,if=none,format=raw,file=null-co://"). \
+                add_drive_raw("id=drive1,if=none,format=raw,file=null-co://")
+        self.vm.launch()
+        result = self.vm.qmp('blockdev-backup', device='drive0',
+                             target='drive1', sync='full')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.vm.shutdown()
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index 42314e9..4fd1c2d 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-........................
+.........................
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
 
 OK
-- 
2.8.2

Re: [Qemu-devel] [PATCH v2 2/2] iotests: Add regression test for drive-backup cluster size error

[Kevin Wolf]

Am 19.05.2016 um 03:25 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [Qemu-stable] [PATCH v2 0/2] backup: Fail early if cannot determine cluster size

[Fam Zheng]

On Thu, 05/19 09:25, Fam Zheng wrote:
> v2: More accurately, s:hang:trigger assertion failure: in the bug fix patch
>     commit message.
>     Add John's and Jeff's R-b.
>     Add a test. [Eric]

Jeff, will you take this series in your tree?

Fam

Re: [Qemu-devel] [PATCH v2 0/2] backup: Fail early if cannot determine cluster size

[Jeff Cody]

On Thu, May 19, 2016 at 09:25:45AM +0800, Fam Zheng wrote:
> v2: More accurately, s:hang:trigger assertion failure: in the bug fix patch
>     commit message.
>     Add John's and Jeff's R-b.
>     Add a test. [Eric]
> 
> 
> 
> Fam Zheng (2):
>   backup: Fail early if cannot determine cluster size
>   iotests: Add regression test for drive-backup cluster size error
> 
>  block/backup.c             | 34 ++++++++++++++++++----------------
>  tests/qemu-iotests/055     | 13 +++++++++++++
>  tests/qemu-iotests/055.out |  4 ++--
>  3 files changed, 33 insertions(+), 18 deletions(-)
> 
> -- 
> 2.8.2
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff