[Qemu-devel] [PATCH for-2.11 0/2] block: Fix permissions in image activation

[Qemu-devel] [PATCH for-2.11 0/2] block: Fix permissions in image activation

[Kevin Wolf]

Kevin Wolf (1):
  block: Fix permissions in image activation

Vladimir Sementsov-Ogievskiy (1):
  iotests: test clearing unknown autoclear_features by qcow2

 block.c                    | 32 +++++++++++++++-------
 tests/qemu-iotests/196     | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/196.out |  5 ++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 94 insertions(+), 10 deletions(-)
 create mode 100755 tests/qemu-iotests/196
 create mode 100644 tests/qemu-iotests/196.out

-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 1/2] block: Fix permissions in image activation

[Kevin Wolf]

Inactive images generally request less permissions for their image files
than they would if they were active (in particular, write permissions).
Activating the image involves extending the permissions, therefore.

drv->bdrv_invalidate_cache() can already require write access to the
image file, so we have to update the permissions earlier than that.
The current code does it only later, so we have to move up this part.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index edc5bb9f9b..6fe5b91260 100644
--- a/block.c
+++ b/block.c
@@ -4178,7 +4178,29 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
         }
     }
 
+    /*
+     * Update permissions, they may differ for inactive nodes.
+     *
+     * Note that the required permissions of inactive images are always a
+     * subset of the permissions required after activating the image. This
+     * allows us to just get the permissions upfront without restricting
+     * drv->bdrv_invalidate_cache().
+     *
+     * It also means that in error cases, we don't have to try and revert to
+     * the old permissions (which is an operation that could fail, too). We can
+     * just keep the extended permissions for the next time that an activation
+     * of the image is tried.
+     */
     bs->open_flags &= ~BDRV_O_INACTIVE;
+    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
+    if (ret < 0) {
+        bs->open_flags |= BDRV_O_INACTIVE;
+        error_propagate(errp, local_err);
+        return;
+    }
+    bdrv_set_perm(bs, perm, shared_perm);
+
     if (bs->drv->bdrv_invalidate_cache) {
         bs->drv->bdrv_invalidate_cache(bs, &local_err);
         if (local_err) {
@@ -4195,16 +4217,6 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
         return;
     }
 
-    /* Update permissions, they may differ for inactive nodes */
-    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
-    if (ret < 0) {
-        bs->open_flags |= BDRV_O_INACTIVE;
-        error_propagate(errp, local_err);
-        return;
-    }
-    bdrv_set_perm(bs, perm, shared_perm);
-
     QLIST_FOREACH(parent, &bs->parents, next_parent) {
         if (parent->role->activate) {
             parent->role->activate(parent, &local_err);
-- 
2.13.6

Re: [Qemu-devel] [PATCH for-2.11 1/2] block: Fix permissions in image activation

[Max Reitz]

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

On 2017-11-17 16:47, Kevin Wolf wrote:
> Inactive images generally request less permissions for their image files
> than they would if they were active (in particular, write permissions).
> Activating the image involves extending the permissions, therefore.
> 
> drv->bdrv_invalidate_cache() can already require write access to the
> image file, so we have to update the permissions earlier than that.
> The current code does it only later, so we have to move up this part.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)

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


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

[Qemu-devel] [PATCH for-2.11 2/2] iotests: test clearing unknown autoclear_features by qcow2

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Test clearing unknown autoclear_features by qcow2 on incoming
migration.

[ kwolf: Fixed wait for destination VM startup ]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/196     | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/196.out |  5 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 72 insertions(+)
 create mode 100755 tests/qemu-iotests/196
 create mode 100644 tests/qemu-iotests/196.out

diff --git a/tests/qemu-iotests/196 b/tests/qemu-iotests/196
new file mode 100755
index 0000000000..4116ebc92b
--- /dev/null
+++ b/tests/qemu-iotests/196
@@ -0,0 +1,66 @@
+#!/usr/bin/env python
+#
+# Test clearing unknown autoclear_features flag by qcow2 after
+# migration. This test mimics migration to older qemu.
+#
+# Copyright (c) 2017 Parallels International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+migfile = os.path.join(iotests.test_dir, 'migfile')
+
+class TestInvalidateAutoclear(iotests.QMPTestCase):
+
+    def tearDown(self):
+        self.vm_a.shutdown()
+        self.vm_b.shutdown()
+        os.remove(disk)
+        os.remove(migfile)
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
+
+        self.vm_a = iotests.VM(path_suffix='a').add_drive(disk)
+        self.vm_a.launch()
+
+        self.vm_b = iotests.VM(path_suffix='b').add_drive(disk)
+        self.vm_b.add_incoming("exec: cat '" + migfile + "'")
+
+    def test_migration(self):
+        result = self.vm_a.qmp('migrate', uri='exec:cat>' + migfile)
+        self.assert_qmp(result, 'return', {});
+        self.assertNotEqual(self.vm_a.event_wait("STOP"), None)
+
+        with open(disk, 'r+b') as f:
+            f.seek(88) # first byte of autoclear_features field
+            f.write(b'\xff')
+
+        self.vm_b.launch()
+        while True:
+            result = self.vm_b.qmp('query-status')
+            if result['return']['status'] == 'running':
+                break
+
+        with open(disk, 'rb') as f:
+            f.seek(88)
+            self.assertEqual(f.read(1), b'\x00')
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/196.out b/tests/qemu-iotests/196.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/196.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 83b839bbe3..1fad602152 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -193,5 +193,6 @@
 192 rw auto quick
 194 rw auto migration quick
 195 rw auto quick
+196 rw auto quick
 197 rw auto quick
 198 rw auto
-- 
2.13.6

Re: [Qemu-devel] [PATCH for-2.11 2/2] iotests: test clearing unknown autoclear_features by qcow2

[Max Reitz]

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

On 2017-11-17 16:47, Kevin Wolf wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Test clearing unknown autoclear_features by qcow2 on incoming
> migration.
> 
> [ kwolf: Fixed wait for destination VM startup ]
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/196     | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/196.out |  5 ++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 72 insertions(+)
>  create mode 100755 tests/qemu-iotests/196
>  create mode 100644 tests/qemu-iotests/196.out
> 
> diff --git a/tests/qemu-iotests/196 b/tests/qemu-iotests/196
> new file mode 100755
> index 0000000000..4116ebc92b
> --- /dev/null
> +++ b/tests/qemu-iotests/196
> @@ -0,0 +1,66 @@
> +#!/usr/bin/env python
> +#
> +# Test clearing unknown autoclear_features flag by qcow2 after
> +# migration. This test mimics migration to older qemu.
> +#
> +# Copyright (c) 2017 Parallels International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import iotests
> +from iotests import qemu_img
> +
> +disk = os.path.join(iotests.test_dir, 'disk')
> +migfile = os.path.join(iotests.test_dir, 'migfile')
> +
> +class TestInvalidateAutoclear(iotests.QMPTestCase):
> +
> +    def tearDown(self):
> +        self.vm_a.shutdown()
> +        self.vm_b.shutdown()
> +        os.remove(disk)
> +        os.remove(migfile)
> +
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
> +
> +        self.vm_a = iotests.VM(path_suffix='a').add_drive(disk)
> +        self.vm_a.launch()
> +
> +        self.vm_b = iotests.VM(path_suffix='b').add_drive(disk)
> +        self.vm_b.add_incoming("exec: cat '" + migfile + "'")
> +
> +    def test_migration(self):
> +        result = self.vm_a.qmp('migrate', uri='exec:cat>' + migfile)
> +        self.assert_qmp(result, 'return', {});
> +        self.assertNotEqual(self.vm_a.event_wait("STOP"), None)
> +
> +        with open(disk, 'r+b') as f:
> +            f.seek(88) # first byte of autoclear_features field
> +            f.write(b'\xff')
> +
> +        self.vm_b.launch()
> +        while True:
> +            result = self.vm_b.qmp('query-status')
> +            if result['return']['status'] == 'running':
> +                break
> +
> +        with open(disk, 'rb') as f:
> +            f.seek(88)
> +            self.assertEqual(f.read(1), b'\x00')

I guess I'd rather shutdown the VM first and do the check then, but
since the header is updated on-disk when opening the qcow2 file...

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

> +
> +if __name__ == '__main__':
> +    iotests.main(supported_fmts=['qcow2'])
> diff --git a/tests/qemu-iotests/196.out b/tests/qemu-iotests/196.out
> new file mode 100644
> index 0000000000..ae1213e6f8
> --- /dev/null
> +++ b/tests/qemu-iotests/196.out
> @@ -0,0 +1,5 @@
> +.
> +----------------------------------------------------------------------
> +Ran 1 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 83b839bbe3..1fad602152 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -193,5 +193,6 @@
>  192 rw auto quick
>  194 rw auto migration quick
>  195 rw auto quick
> +196 rw auto quick
>  197 rw auto quick
>  198 rw auto
> 



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