[Qemu-devel] [PATCH 38/54] backup: Use real permissions in backup block job

Kevin Wolf posted 54 patches 8 years, 11 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 38/54] backup: Use real permissions in backup block job
Posted by Kevin Wolf 8 years, 11 months ago
The backup block job doesn't have very complicated requirements: It
needs to read from the source and write to the target, but it's fine
with either side being changed. The only restriction is that we can't
resize the image because the job uses a cached value.

qemu-iotests 055 needs to be changed because it used a target which was
already attached to a virtio-blk device. The permission system correctly
forbids this (virtio-blk can't accept another writer with its default
share-rw=off).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c         | 15 ++++++++++-----
 tests/qemu-iotests/055 | 11 +++++++----
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 405f271..d1ab617 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -618,15 +618,20 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
-    /* FIXME Use real permissions */
-    job = block_job_create(job_id, &backup_job_driver, bs, 0, BLK_PERM_ALL,
+    /* job->common.len is fixed, so we can't allow resize */
+    job = block_job_create(job_id, &backup_job_driver, bs,
+                           BLK_PERM_CONSISTENT_READ,
+                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
+                           BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD,
                            speed, creation_flags, cb, opaque, errp);
     if (!job) {
         goto error;
     }
 
-    /* FIXME Use real permissions */
-    job->target = blk_new(0, BLK_PERM_ALL);
+    /* The target must match the source in size, so no resize here either */
+    job->target = blk_new(BLK_PERM_WRITE,
+                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
+                          BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD);
     ret = blk_insert_bs(job->target, target, errp);
     if (ret < 0) {
         goto error;
@@ -657,7 +662,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
     }
 
-    /* FIXME Use real permissions */
+    /* Required permissions are already taken with target's blk_new() */
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
                        &error_abort);
     job->common.len = len;
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 1d3fd04..aafcd24 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,7 +48,8 @@ class TestSingleDrive(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.add_drive(blockdev_target_img, interface="none")
         if iotests.qemu_default_machine == 'pc':
             self.vm.add_drive(None, 'media=cdrom', 'ide')
         self.vm.launch()
@@ -164,7 +165,8 @@ class TestSetSpeed(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.add_drive(blockdev_target_img, interface="none")
         self.vm.launch()
 
     def tearDown(self):
@@ -247,7 +249,8 @@ class TestSingleTransaction(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.add_drive(blockdev_target_img, interface="none")
         if iotests.qemu_default_machine == 'pc':
             self.vm.add_drive(None, 'media=cdrom', 'ide')
         self.vm.launch()
@@ -460,7 +463,7 @@ class TestDriveCompression(iotests.QMPTestCase):
 
         qemu_img('create', '-f', fmt, blockdev_target_img,
                  str(TestDriveCompression.image_len), *args)
-        self.vm.add_drive(blockdev_target_img, format=fmt)
+        self.vm.add_drive(blockdev_target_img, format=fmt, interface="none")
 
         self.vm.launch()
 
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH 38/54] backup: Use real permissions in backup block job
Posted by Max Reitz 8 years, 11 months ago
On 21.02.2017 15:58, Kevin Wolf wrote:
> The backup block job doesn't have very complicated requirements: It
> needs to read from the source and write to the target, but it's fine
> with either side being changed. The only restriction is that we can't
> resize the image because the job uses a cached value.
> 
> qemu-iotests 055 needs to be changed because it used a target which was
> already attached to a virtio-blk device. The permission system correctly
> forbids this (virtio-blk can't accept another writer with its default
> share-rw=off).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c         | 15 ++++++++++-----
>  tests/qemu-iotests/055 | 11 +++++++----
>  2 files changed, 17 insertions(+), 9 deletions(-)

Not sure how much sense it makes to allow other writers on the target
node, but it's not as if the backup block job itself would care:

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