[PATCH 0/2] backup-top failure path fix

[PATCH 0/2] backup-top failure path fix

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a small crash fix.

Vladimir Sementsov-Ogievskiy (2):
  block/backup-top: fix failure path
  iotests: add test for backup-top failure on permission activation

 block/backup-top.c         | 21 ++++++-----
 tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/283.out |  8 ++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 96 insertions(+), 9 deletions(-)
 create mode 100644 tests/qemu-iotests/283
 create mode 100644 tests/qemu-iotests/283.out

-- 
2.21.0

[PATCH 1/2] block/backup-top: fix failure path

[Vladimir Sementsov-Ogievskiy]

We can't access top after call bdrv_backup_top_drop, as it is already
freed at this time.

Also, no needs to unref target child by hand, it will be unrefed on
bdrv_close() automatically.

So, just do bdrv_backup_top_drop if append succeed and one bdrv_unref
otherwise.

Note, that in !appended case bdrv_unref(top) moved into drained section
on source. It doesn't really matter, but just for code simplicity.

Fixes: 7df7868b96404
Cc: qemu-stable@nongnu.org # v4.2.0
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup-top.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index 818d3f26b4..19f18d541b 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -190,6 +190,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
                                                  filter_node_name,
                                                  BDRV_O_RDWR, errp);
+    bool appended = false;
 
     if (!top) {
         return NULL;
@@ -212,8 +213,9 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     bdrv_append(top, source, &local_err);
     if (local_err) {
         error_prepend(&local_err, "Cannot append backup-top filter: ");
-        goto append_failed;
+        goto fail;
     }
+    appended = true;
 
     /*
      * bdrv_append() finished successfully, now we can require permissions
@@ -224,14 +226,14 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     if (local_err) {
         error_prepend(&local_err,
                       "Cannot set permissions for backup-top filter: ");
-        goto failed_after_append;
+        goto fail;
     }
 
     state->bcs = block_copy_state_new(top->backing, state->target,
                                       cluster_size, write_flags, &local_err);
     if (local_err) {
         error_prepend(&local_err, "Cannot create block-copy-state: ");
-        goto failed_after_append;
+        goto fail;
     }
     *bcs = state->bcs;
 
@@ -239,14 +241,15 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
 
     return top;
 
-failed_after_append:
-    state->active = false;
-    bdrv_backup_top_drop(top);
+fail:
+    if (appended) {
+        state->active = false;
+        bdrv_backup_top_drop(top);
+    } else {
+        bdrv_unref(top);
+    }
 
-append_failed:
     bdrv_drained_end(source);
-    bdrv_unref_child(top, state->target);
-    bdrv_unref(top);
     error_propagate(errp, local_err);
 
     return NULL;
-- 
2.21.0

Re: [PATCH 1/2] block/backup-top: fix failure path

[Max Reitz]

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

On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
> We can't access top after call bdrv_backup_top_drop, as it is already
> freed at this time.
> 
> Also, no needs to unref target child by hand, it will be unrefed on
> bdrv_close() automatically.
> 
> So, just do bdrv_backup_top_drop if append succeed and one bdrv_unref
> otherwise.
> 
> Note, that in !appended case bdrv_unref(top) moved into drained section
> on source. It doesn't really matter, but just for code simplicity.
> 
> Fixes: 7df7868b96404
> Cc: qemu-stable@nongnu.org # v4.2.0
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup-top.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

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


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

[PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

This test checks that bug is really fixed by previous commit.

Cc: qemu-stable@nongnu.org # v4.2.0
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/283.out |  8 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 84 insertions(+)
 create mode 100644 tests/qemu-iotests/283
 create mode 100644 tests/qemu-iotests/283.out

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
new file mode 100644
index 0000000000..f0f216d109
--- /dev/null
+++ b/tests/qemu-iotests/283
@@ -0,0 +1,75 @@
+#!/usr/bin/env python
+#
+# Test for backup-top filter permission activation failure
+#
+# Copyright (c) 2019 Virtuozzo 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 iotests
+
+# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+size = 1024 * 1024
+
+"""
+On activation, backup-top is going to unshare write permission on its
+source child. It will be impossible for the following configuration:
+
+    ┌────────┐  target  ┌─────────────┐
+    │ target │ ◀─────── │ backup_top  │
+    └────────┘          └─────────────┘
+                            │
+                            │ backing
+                            ▼
+                        ┌─────────────┐
+                        │   source    │
+                        └─────────────┘
+                            │
+                            │ file
+                            ▼
+                        ┌─────────────┐  write perm   ┌───────┐
+                        │    base     │ ◀──────────── │ other │
+                        └─────────────┘               └───────┘
+
+Write unsharing will be propagated to the "source->base"link and will
+conflict with other node write permission.
+
+(Note, that we can't just consider source to be direct child of other,
+as in this case this link will be broken, when backup_top is appended)
+"""
+
+vm = iotests.VM()
+vm.launch()
+
+vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
+
+vm.qmp_log('blockdev-add', **{
+    'node-name': 'source',
+    'driver': 'blkdebug',
+    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
+})
+
+vm.qmp_log('blockdev-add', **{
+    'node-name': 'other',
+    'driver': 'blkdebug',
+    'image': 'base',
+    'take-child-perms': ['write']
+})
+
+vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
new file mode 100644
index 0000000000..daaf5828c1
--- /dev/null
+++ b/tests/qemu-iotests/283.out
@@ -0,0 +1,8 @@
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}}
+{"error": {"class": "GenericError", "desc": "Cannot set permissions for backup-top filter: Conflicts with use by other as 'image', which uses 'write' on base"}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cb2b789e44..d827e8c821 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -288,3 +288,4 @@
 277 rw quick
 279 rw backing quick
 280 rw migration quick
+283 auto quick
-- 
2.21.0

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
> This test checks that bug is really fixed by previous commit.
> 
> Cc: qemu-stable@nongnu.org # v4.2.0
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/283.out |  8 ++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 84 insertions(+)
>  create mode 100644 tests/qemu-iotests/283
>  create mode 100644 tests/qemu-iotests/283.out

The test looks good to me, I just have a comment nit and a note on the
fact that this should probably be queued only after Thomas’s “Enable
more iotests during "make check-block"” series.

> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
> new file mode 100644
> index 0000000000..f0f216d109
> --- /dev/null
> +++ b/tests/qemu-iotests/283
> @@ -0,0 +1,75 @@
> +#!/usr/bin/env python
> +#
> +# Test for backup-top filter permission activation failure
> +#
> +# Copyright (c) 2019 Virtuozzo 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 iotests
> +
> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +size = 1024 * 1024
> +
> +"""
> +On activation, backup-top is going to unshare write permission on its
> +source child. It will be impossible for the following configuration:

“The following configuration will become impossible”?

I think there should be some note that this is exactly what we want to
test, i.e. what happens when this impossible configuration is attempted
by starting a backup.  (And maybe why this isn’t allowed; namely because
we couldn’t do CBW for such write accesses.)

> +
> +    ┌────────┐  target  ┌─────────────┐
> +    │ target │ ◀─────── │ backup_top  │
> +    └────────┘          └─────────────┘
> +                            │
> +                            │ backing
> +                            ▼
> +                        ┌─────────────┐
> +                        │   source    │
> +                        └─────────────┘
> +                            │
> +                            │ file
> +                            ▼
> +                        ┌─────────────┐  write perm   ┌───────┐
> +                        │    base     │ ◀──────────── │ other │
> +                        └─────────────┘               └───────┘

Cool Unicode art. :-)

> +
> +Write unsharing will be propagated to the "source->base"link and will
> +conflict with other node write permission.
> +
> +(Note, that we can't just consider source to be direct child of other,
> +as in this case this link will be broken, when backup_top is appended)
> +"""
> +
> +vm = iotests.VM()
> +vm.launch()
> +
> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
> +
> +vm.qmp_log('blockdev-add', **{
> +    'node-name': 'source',
> +    'driver': 'blkdebug',
> +    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
> +})
> +
> +vm.qmp_log('blockdev-add', **{
> +    'node-name': 'other',
> +    'driver': 'blkdebug',
> +    'image': 'base',
> +    'take-child-perms': ['write']
> +})
> +
> +vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
> +
> +vm.shutdown()

[...]

> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index cb2b789e44..d827e8c821 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -288,3 +288,4 @@
>  277 rw quick
>  279 rw backing quick
>  280 rw migration quick
> +283 auto quick

Hm.  This would be the first Python test in auto.  Thomas’s series has
at least one patch that seems useful to come before we do this, namely
“Skip Python-based tests if QEMU does not support virtio-blk”.  So I
suppose his series should come before this, then.

Max


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

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

20.01.2020 20:04, Max Reitz wrote:
> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>> This test checks that bug is really fixed by previous commit.
>>
>> Cc: qemu-stable@nongnu.org # v4.2.0
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/283.out |  8 ++++
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 84 insertions(+)
>>   create mode 100644 tests/qemu-iotests/283
>>   create mode 100644 tests/qemu-iotests/283.out
> 
> The test looks good to me, I just have a comment nit and a note on the
> fact that this should probably be queued only after Thomas’s “Enable
> more iotests during "make check-block"” series.
> 
>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>> new file mode 100644
>> index 0000000000..f0f216d109
>> --- /dev/null
>> +++ b/tests/qemu-iotests/283
>> @@ -0,0 +1,75 @@
>> +#!/usr/bin/env python
>> +#
>> +# Test for backup-top filter permission activation failure
>> +#
>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>> +
>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>> +
>> +size = 1024 * 1024
>> +
>> +"""
>> +On activation, backup-top is going to unshare write permission on its
>> +source child. It will be impossible for the following configuration:
> 
> “The following configuration will become impossible”?

Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
is impossible with such configuration..

> 
> I think there should be some note that this is exactly what we want to
> test, i.e. what happens when this impossible configuration is attempted
> by starting a backup.  (And maybe why this isn’t allowed; namely because
> we couldn’t do CBW for such write accesses.)
> 
>> +
>> +    ┌────────┐  target  ┌─────────────┐
>> +    │ target │ ◀─────── │ backup_top  │
>> +    └────────┘          └─────────────┘
>> +                            │
>> +                            │ backing
>> +                            ▼
>> +                        ┌─────────────┐
>> +                        │   source    │
>> +                        └─────────────┘
>> +                            │
>> +                            │ file
>> +                            ▼
>> +                        ┌─────────────┐  write perm   ┌───────┐
>> +                        │    base     │ ◀──────────── │ other │
>> +                        └─────────────┘               └───────┘
> 
> Cool Unicode art. :-)

I found the great tool: https://dot-to-ascii.ggerganov.com/

> 
>> +
>> +Write unsharing will be propagated to the "source->base"link and will
>> +conflict with other node write permission.
>> +
>> +(Note, that we can't just consider source to be direct child of other,
>> +as in this case this link will be broken, when backup_top is appended)
>> +"""
>> +
>> +vm = iotests.VM()
>> +vm.launch()
>> +
>> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
>> +
>> +vm.qmp_log('blockdev-add', **{
>> +    'node-name': 'source',
>> +    'driver': 'blkdebug',
>> +    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
>> +})
>> +
>> +vm.qmp_log('blockdev-add', **{
>> +    'node-name': 'other',
>> +    'driver': 'blkdebug',
>> +    'image': 'base',
>> +    'take-child-perms': ['write']
>> +})
>> +
>> +vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
>> +
>> +vm.shutdown()
> 
> [...]
> 
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index cb2b789e44..d827e8c821 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -288,3 +288,4 @@
>>   277 rw quick
>>   279 rw backing quick
>>   280 rw migration quick
>> +283 auto quick
> 
> Hm.  This would be the first Python test in auto.

Missed that. It's OK to define it just "quick" and update later.

>  Thomas’s series has
> at least one patch that seems useful to come before we do this, namely
> “Skip Python-based tests if QEMU does not support virtio-blk”.  So I
> suppose his series should come before this, then.
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
> 20.01.2020 20:04, Max Reitz wrote:
>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>> This test checks that bug is really fixed by previous commit.
>>>
>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/283.out |  8 ++++
>>>   tests/qemu-iotests/group   |  1 +
>>>   3 files changed, 84 insertions(+)
>>>   create mode 100644 tests/qemu-iotests/283
>>>   create mode 100644 tests/qemu-iotests/283.out
>>
>> The test looks good to me, I just have a comment nit and a note on the
>> fact that this should probably be queued only after Thomas’s “Enable
>> more iotests during "make check-block"” series.
>>
>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>> new file mode 100644
>>> index 0000000000..f0f216d109
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/283
>>> @@ -0,0 +1,75 @@
>>> +#!/usr/bin/env python
>>> +#
>>> +# Test for backup-top filter permission activation failure
>>> +#
>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>> +
>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>> +
>>> +size = 1024 * 1024
>>> +
>>> +"""
>>> +On activation, backup-top is going to unshare write permission on its
>>> +source child. It will be impossible for the following configuration:
>>
>> “The following configuration will become impossible”?
> 
> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
> is impossible with such configuration..

But backup_top always unshares the write permission on the source.

>> I think there should be some note that this is exactly what we want to
>> test, i.e. what happens when this impossible configuration is attempted
>> by starting a backup.  (And maybe why this isn’t allowed; namely because
>> we couldn’t do CBW for such write accesses.)
>>
>>> +
>>> +    ┌────────┐  target  ┌─────────────┐
>>> +    │ target │ ◀─────── │ backup_top  │
>>> +    └────────┘          └─────────────┘
>>> +                            │
>>> +                            │ backing
>>> +                            ▼
>>> +                        ┌─────────────┐
>>> +                        │   source    │
>>> +                        └─────────────┘
>>> +                            │
>>> +                            │ file
>>> +                            ▼
>>> +                        ┌─────────────┐  write perm   ┌───────┐
>>> +                        │    base     │ ◀──────────── │ other │
>>> +                        └─────────────┘               └───────┘
>>
>> Cool Unicode art. :-)
> 
> I found the great tool: https://dot-to-ascii.ggerganov.com/

Thanks!

Max

>>> +
>>> +Write unsharing will be propagated to the "source->base"link and will
>>> +conflict with other node write permission.
>>> +
>>> +(Note, that we can't just consider source to be direct child of other,
>>> +as in this case this link will be broken, when backup_top is appended)
>>> +"""
>>> +
>>> +vm = iotests.VM()
>>> +vm.launch()
>>> +
>>> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
>>> +
>>> +vm.qmp_log('blockdev-add', **{
>>> +    'node-name': 'source',
>>> +    'driver': 'blkdebug',
>>> +    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
>>> +})
>>> +
>>> +vm.qmp_log('blockdev-add', **{
>>> +    'node-name': 'other',
>>> +    'driver': 'blkdebug',
>>> +    'image': 'base',
>>> +    'take-child-perms': ['write']
>>> +})
>>> +
>>> +vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
>>> +
>>> +vm.shutdown()
>>
>> [...]
>>
>>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>>> index cb2b789e44..d827e8c821 100644
>>> --- a/tests/qemu-iotests/group
>>> +++ b/tests/qemu-iotests/group
>>> @@ -288,3 +288,4 @@
>>>   277 rw quick
>>>   279 rw backing quick
>>>   280 rw migration quick
>>> +283 auto quick
>>
>> Hm.  This would be the first Python test in auto.
> 
> Missed that. It's OK to define it just "quick" and update later.
> 
>>  Thomas’s series has
>> at least one patch that seems useful to come before we do this, namely
>> “Skip Python-based tests if QEMU does not support virtio-blk”.  So I
>> suppose his series should come before this, then.
>>
>> Max
>>
> 
> 



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

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 12:14, Max Reitz wrote:
> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>> 20.01.2020 20:04, Max Reitz wrote:
>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>> This test checks that bug is really fixed by previous commit.
>>>>
>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>    tests/qemu-iotests/283.out |  8 ++++
>>>>    tests/qemu-iotests/group   |  1 +
>>>>    3 files changed, 84 insertions(+)
>>>>    create mode 100644 tests/qemu-iotests/283
>>>>    create mode 100644 tests/qemu-iotests/283.out
>>>
>>> The test looks good to me, I just have a comment nit and a note on the
>>> fact that this should probably be queued only after Thomas’s “Enable
>>> more iotests during "make check-block"” series.
>>>
>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>> new file mode 100644
>>>> index 0000000000..f0f216d109
>>>> --- /dev/null
>>>> +++ b/tests/qemu-iotests/283
>>>> @@ -0,0 +1,75 @@
>>>> +#!/usr/bin/env python
>>>> +#
>>>> +# Test for backup-top filter permission activation failure
>>>> +#
>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>> +
>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>> +
>>>> +size = 1024 * 1024
>>>> +
>>>> +"""
>>>> +On activation, backup-top is going to unshare write permission on its
>>>> +source child. It will be impossible for the following configuration:
>>>
>>> “The following configuration will become impossible”?
>>
>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>> is impossible with such configuration..
> 
> But backup_top always unshares the write permission on the source.

Yes, and I just try to say, that this action will fail. And the test checks that it
fails (and it crashes with current master instead of fail).

> 
>>> I think there should be some note that this is exactly what we want to
>>> test, i.e. what happens when this impossible configuration is attempted
>>> by starting a backup.  (And maybe why this isn’t allowed; namely because
>>> we couldn’t do CBW for such write accesses.)
>>>
>>>> +
>>>> +    ┌────────┐  target  ┌─────────────┐
>>>> +    │ target │ ◀─────── │ backup_top  │
>>>> +    └────────┘          └─────────────┘
>>>> +                            │
>>>> +                            │ backing
>>>> +                            ▼
>>>> +                        ┌─────────────┐
>>>> +                        │   source    │
>>>> +                        └─────────────┘
>>>> +                            │
>>>> +                            │ file
>>>> +                            ▼
>>>> +                        ┌─────────────┐  write perm   ┌───────┐
>>>> +                        │    base     │ ◀──────────── │ other │
>>>> +                        └─────────────┘               └───────┘
>>>
>>> Cool Unicode art. :-)
>>
>> I found the great tool: https://dot-to-ascii.ggerganov.com/
> 
> Thanks!
> 
> Max
> 
>>>> +
>>>> +Write unsharing will be propagated to the "source->base"link and will
>>>> +conflict with other node write permission.
>>>> +
>>>> +(Note, that we can't just consider source to be direct child of other,
>>>> +as in this case this link will be broken, when backup_top is appended)
>>>> +"""
>>>> +
>>>> +vm = iotests.VM()
>>>> +vm.launch()
>>>> +
>>>> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
>>>> +
>>>> +vm.qmp_log('blockdev-add', **{
>>>> +    'node-name': 'source',
>>>> +    'driver': 'blkdebug',
>>>> +    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
>>>> +})
>>>> +
>>>> +vm.qmp_log('blockdev-add', **{
>>>> +    'node-name': 'other',
>>>> +    'driver': 'blkdebug',
>>>> +    'image': 'base',
>>>> +    'take-child-perms': ['write']
>>>> +})
>>>> +
>>>> +vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
>>>> +
>>>> +vm.shutdown()
>>>
>>> [...]
>>>
>>>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>>>> index cb2b789e44..d827e8c821 100644
>>>> --- a/tests/qemu-iotests/group
>>>> +++ b/tests/qemu-iotests/group
>>>> @@ -288,3 +288,4 @@
>>>>    277 rw quick
>>>>    279 rw backing quick
>>>>    280 rw migration quick
>>>> +283 auto quick
>>>
>>> Hm.  This would be the first Python test in auto.
>>
>> Missed that. It's OK to define it just "quick" and update later.
>>
>>>   Thomas’s series has
>>> at least one patch that seems useful to come before we do this, namely
>>> “Skip Python-based tests if QEMU does not support virtio-blk”.  So I
>>> suppose his series should come before this, then.
>>>
>>> Max
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 12:14, Max Reitz wrote:
>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.01.2020 20:04, Max Reitz wrote:
>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>> This test checks that bug is really fixed by previous commit.
>>>>>
>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>    tests/qemu-iotests/283.out |  8 ++++
>>>>>    tests/qemu-iotests/group   |  1 +
>>>>>    3 files changed, 84 insertions(+)
>>>>>    create mode 100644 tests/qemu-iotests/283
>>>>>    create mode 100644 tests/qemu-iotests/283.out
>>>>
>>>> The test looks good to me, I just have a comment nit and a note on the
>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>> more iotests during "make check-block"” series.
>>>>
>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>> new file mode 100644
>>>>> index 0000000000..f0f216d109
>>>>> --- /dev/null
>>>>> +++ b/tests/qemu-iotests/283
>>>>> @@ -0,0 +1,75 @@
>>>>> +#!/usr/bin/env python
>>>>> +#
>>>>> +# Test for backup-top filter permission activation failure
>>>>> +#
>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>> +
>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>> +
>>>>> +size = 1024 * 1024
>>>>> +
>>>>> +"""
>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>> +source child. It will be impossible for the following configuration:
>>>>
>>>> “The following configuration will become impossible”?
>>>
>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>> is impossible with such configuration..
>>
>> But backup_top always unshares the write permission on the source.
> 
> Yes, and I just try to say, that this action will fail. And the test checks that it
> fails (and it crashes with current master instead of fail).

OK.  So what I was trying to say is that the comment currently only
states that this will fail.  I’d prefer it to also reassure me that it’s
correct that this fails (because all writes on the backup source must go
through backup_top), and that this is exactly what we want to test here.

On first reading, I was wondering why exactly this comment would tell me
all these things, because I didn’t know what the test wants to test in
the first place.

Max

>>>> I think there should be some note that this is exactly what we want to
>>>> test, i.e. what happens when this impossible configuration is attempted
>>>> by starting a backup.  (And maybe why this isn’t allowed; namely because
>>>> we couldn’t do CBW for such write accesses.)
>>>>
>>>>> +
>>>>> +    ┌────────┐  target  ┌─────────────┐
>>>>> +    │ target │ ◀─────── │ backup_top  │
>>>>> +    └────────┘          └─────────────┘
>>>>> +                            │
>>>>> +                            │ backing
>>>>> +                            ▼
>>>>> +                        ┌─────────────┐
>>>>> +                        │   source    │
>>>>> +                        └─────────────┘
>>>>> +                            │
>>>>> +                            │ file
>>>>> +                            ▼
>>>>> +                        ┌─────────────┐  write perm   ┌───────┐
>>>>> +                        │    base     │ ◀──────────── │ other │
>>>>> +                        └─────────────┘               └───────┘
>>>>
>>>> Cool Unicode art. :-)
>>>
>>> I found the great tool: https://dot-to-ascii.ggerganov.com/
>>
>> Thanks!
>>
>> Max
>>
>>>>> +
>>>>> +Write unsharing will be propagated to the "source->base"link and will
>>>>> +conflict with other node write permission.
>>>>> +
>>>>> +(Note, that we can't just consider source to be direct child of other,
>>>>> +as in this case this link will be broken, when backup_top is appended)
>>>>> +"""
>>>>> +
>>>>> +vm = iotests.VM()
>>>>> +vm.launch()
>>>>> +
>>>>> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
>>>>> +
>>>>> +vm.qmp_log('blockdev-add', **{
>>>>> +    'node-name': 'source',
>>>>> +    'driver': 'blkdebug',
>>>>> +    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
>>>>> +})
>>>>> +
>>>>> +vm.qmp_log('blockdev-add', **{
>>>>> +    'node-name': 'other',
>>>>> +    'driver': 'blkdebug',
>>>>> +    'image': 'base',
>>>>> +    'take-child-perms': ['write']
>>>>> +})
>>>>> +
>>>>> +vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
>>>>> +
>>>>> +vm.shutdown()
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>>>>> index cb2b789e44..d827e8c821 100644
>>>>> --- a/tests/qemu-iotests/group
>>>>> +++ b/tests/qemu-iotests/group
>>>>> @@ -288,3 +288,4 @@
>>>>>    277 rw quick
>>>>>    279 rw backing quick
>>>>>    280 rw migration quick
>>>>> +283 auto quick
>>>>
>>>> Hm.  This would be the first Python test in auto.
>>>
>>> Missed that. It's OK to define it just "quick" and update later.
>>>
>>>>   Thomas’s series has
>>>> at least one patch that seems useful to come before we do this, namely
>>>> “Skip Python-based tests if QEMU does not support virtio-blk”.  So I
>>>> suppose his series should come before this, then.
>>>>
>>>> Max
>>>>
>>>
>>>
>>
>>
> 
> 



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

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 12:41, Max Reitz wrote:
> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:14, Max Reitz wrote:
>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>
>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>     tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>     tests/qemu-iotests/283.out |  8 ++++
>>>>>>     tests/qemu-iotests/group   |  1 +
>>>>>>     3 files changed, 84 insertions(+)
>>>>>>     create mode 100644 tests/qemu-iotests/283
>>>>>>     create mode 100644 tests/qemu-iotests/283.out
>>>>>
>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>> more iotests during "make check-block"” series.
>>>>>
>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>> new file mode 100644
>>>>>> index 0000000000..f0f216d109
>>>>>> --- /dev/null
>>>>>> +++ b/tests/qemu-iotests/283
>>>>>> @@ -0,0 +1,75 @@
>>>>>> +#!/usr/bin/env python
>>>>>> +#
>>>>>> +# Test for backup-top filter permission activation failure
>>>>>> +#
>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>> +
>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>> +
>>>>>> +size = 1024 * 1024
>>>>>> +
>>>>>> +"""
>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>> +source child. It will be impossible for the following configuration:
>>>>>
>>>>> “The following configuration will become impossible”?
>>>>
>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>> is impossible with such configuration..
>>>
>>> But backup_top always unshares the write permission on the source.
>>
>> Yes, and I just try to say, that this action will fail. And the test checks that it
>> fails (and it crashes with current master instead of fail).
> 
> OK.  So what I was trying to say is that the comment currently only
> states that this will fail.  I’d prefer it to also reassure me that it’s
> correct that this fails (because all writes on the backup source must go
> through backup_top), and that this is exactly what we want to test here.
> 
> On first reading, I was wondering why exactly this comment would tell me
> all these things, because I didn’t know what the test wants to test in
> the first place.
> 
> Max

Hmm, something like:

Backup wants to copy a point-in-time state of the source node. So, it catches all writes
to the source node by appending backup-top filter above it. So we handle all changes which
comes from source node parents. To prevent appearing of new writing parents during the
progress, backup-top unshares write permission on its source child. This has additional
implication: as this "unsharing" is propagated by default by backing/file children,
backup-top conflicts with any side parents of source sub-tree with write permission.
And this is in good relation with the general idea: with such parents we can't guarantee
point-in-time backup. So, trying to backup the configuration with writing side parents of
source sub-tree nodes should fail. Let's test it.

> 
>>>>> I think there should be some note that this is exactly what we want to
>>>>> test, i.e. what happens when this impossible configuration is attempted
>>>>> by starting a backup.  (And maybe why this isn’t allowed; namely because
>>>>> we couldn’t do CBW for such write accesses.)
>>>>>
>>>>>> +
>>>>>> +    ┌────────┐  target  ┌─────────────┐
>>>>>> +    │ target │ ◀─────── │ backup_top  │
>>>>>> +    └────────┘          └─────────────┘
>>>>>> +                            │
>>>>>> +                            │ backing
>>>>>> +                            ▼
>>>>>> +                        ┌─────────────┐
>>>>>> +                        │   source    │
>>>>>> +                        └─────────────┘
>>>>>> +                            │
>>>>>> +                            │ file
>>>>>> +                            ▼
>>>>>> +                        ┌─────────────┐  write perm   ┌───────┐
>>>>>> +                        │    base     │ ◀──────────── │ other │
>>>>>> +                        └─────────────┘               └───────┘
>>>>>
>>>>> Cool Unicode art. :-)
>>>>
>>>> I found the great tool: https://dot-to-ascii.ggerganov.com/
>>>
>>> Thanks!
>>>
>>> Max
>>>
>>>>>> +
>>>>>> +Write unsharing will be propagated to the "source->base"link and will
>>>>>> +conflict with other node write permission.
>>>>>> +
>>>>>> +(Note, that we can't just consider source to be direct child of other,
>>>>>> +as in this case this link will be broken, when backup_top is appended)
>>>>>> +"""
>>>>>> +
>>>>>> +vm = iotests.VM()
>>>>>> +vm.launch()
>>>>>> +
>>>>>> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
>>>>>> +
>>>>>> +vm.qmp_log('blockdev-add', **{
>>>>>> +    'node-name': 'source',
>>>>>> +    'driver': 'blkdebug',
>>>>>> +    'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
>>>>>> +})
>>>>>> +
>>>>>> +vm.qmp_log('blockdev-add', **{
>>>>>> +    'node-name': 'other',
>>>>>> +    'driver': 'blkdebug',
>>>>>> +    'image': 'base',
>>>>>> +    'take-child-perms': ['write']
>>>>>> +})
>>>>>> +
>>>>>> +vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
>>>>>> +
>>>>>> +vm.shutdown()
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>>>>>> index cb2b789e44..d827e8c821 100644
>>>>>> --- a/tests/qemu-iotests/group
>>>>>> +++ b/tests/qemu-iotests/group
>>>>>> @@ -288,3 +288,4 @@
>>>>>>     277 rw quick
>>>>>>     279 rw backing quick
>>>>>>     280 rw migration quick
>>>>>> +283 auto quick
>>>>>
>>>>> Hm.  This would be the first Python test in auto.
>>>>
>>>> Missed that. It's OK to define it just "quick" and update later.
>>>>
>>>>>    Thomas’s series has
>>>>> at least one patch that seems useful to come before we do this, namely
>>>>> “Skip Python-based tests if QEMU does not support virtio-blk”.  So I
>>>>> suppose his series should come before this, then.
>>>>>
>>>>> Max
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 12:41, Max Reitz wrote:
>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.01.2020 12:14, Max Reitz wrote:
>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>
>>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>> ---
>>>>>>>     tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>>     tests/qemu-iotests/283.out |  8 ++++
>>>>>>>     tests/qemu-iotests/group   |  1 +
>>>>>>>     3 files changed, 84 insertions(+)
>>>>>>>     create mode 100644 tests/qemu-iotests/283
>>>>>>>     create mode 100644 tests/qemu-iotests/283.out
>>>>>>
>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>> more iotests during "make check-block"” series.
>>>>>>
>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..f0f216d109
>>>>>>> --- /dev/null
>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>> @@ -0,0 +1,75 @@
>>>>>>> +#!/usr/bin/env python
>>>>>>> +#
>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>> +#
>>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>>> +
>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>> +
>>>>>>> +size = 1024 * 1024
>>>>>>> +
>>>>>>> +"""
>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>
>>>>>> “The following configuration will become impossible”?
>>>>>
>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>>> is impossible with such configuration..
>>>>
>>>> But backup_top always unshares the write permission on the source.
>>>
>>> Yes, and I just try to say, that this action will fail. And the test checks that it
>>> fails (and it crashes with current master instead of fail).
>>
>> OK.  So what I was trying to say is that the comment currently only
>> states that this will fail.  I’d prefer it to also reassure me that it’s
>> correct that this fails (because all writes on the backup source must go
>> through backup_top), and that this is exactly what we want to test here.
>>
>> On first reading, I was wondering why exactly this comment would tell me
>> all these things, because I didn’t know what the test wants to test in
>> the first place.
>>
>> Max
> 
> Hmm, something like:
> 
> Backup wants to copy a point-in-time state of the source node. So, it catches all writes
> to the source node by appending backup-top filter above it. So we handle all changes which
> comes from source node parents. To prevent appearing of new writing parents during the
> progress, backup-top unshares write permission on its source child. This has additional
> implication: as this "unsharing" is propagated by default by backing/file children,
> backup-top conflicts with any side parents of source sub-tree with write permission.
> And this is in good relation with the general idea: with such parents we can't guarantee
> point-in-time backup.

Works for me (thanks :-)), but a shorter “When performing a backup, all
writes on the source subtree must go through the backup-top filter so it
can copy all data to the target before it is changed.  Therefore,
backup-top cannot allow other nodes to change data on its source child.”
would work for me just as well.

> So, trying to backup the configuration with writing side parents of
> source sub-tree nodes should fail. Let's test it.
Max


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

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 15:39, Max Reitz wrote:
> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:41, Max Reitz wrote:
>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>>> 21.01.2020 12:14, Max Reitz wrote:
>>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>>
>>>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>> ---
>>>>>>>>      tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>      tests/qemu-iotests/283.out |  8 ++++
>>>>>>>>      tests/qemu-iotests/group   |  1 +
>>>>>>>>      3 files changed, 84 insertions(+)
>>>>>>>>      create mode 100644 tests/qemu-iotests/283
>>>>>>>>      create mode 100644 tests/qemu-iotests/283.out
>>>>>>>
>>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>>> more iotests during "make check-block"” series.
>>>>>>>
>>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..f0f216d109
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>>> @@ -0,0 +1,75 @@
>>>>>>>> +#!/usr/bin/env python
>>>>>>>> +#
>>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>>> +#
>>>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>>>> +
>>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>>> +
>>>>>>>> +size = 1024 * 1024
>>>>>>>> +
>>>>>>>> +"""
>>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>>
>>>>>>> “The following configuration will become impossible”?
>>>>>>
>>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>>>> is impossible with such configuration..
>>>>>
>>>>> But backup_top always unshares the write permission on the source.
>>>>
>>>> Yes, and I just try to say, that this action will fail. And the test checks that it
>>>> fails (and it crashes with current master instead of fail).
>>>
>>> OK.  So what I was trying to say is that the comment currently only
>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>> correct that this fails (because all writes on the backup source must go
>>> through backup_top), and that this is exactly what we want to test here.
>>>
>>> On first reading, I was wondering why exactly this comment would tell me
>>> all these things, because I didn’t know what the test wants to test in
>>> the first place.
>>>
>>> Max
>>
>> Hmm, something like:
>>
>> Backup wants to copy a point-in-time state of the source node. So, it catches all writes
>> to the source node by appending backup-top filter above it. So we handle all changes which
>> comes from source node parents. To prevent appearing of new writing parents during the
>> progress, backup-top unshares write permission on its source child. This has additional
>> implication: as this "unsharing" is propagated by default by backing/file children,
>> backup-top conflicts with any side parents of source sub-tree with write permission.
>> And this is in good relation with the general idea: with such parents we can't guarantee
>> point-in-time backup.
> 
> Works for me (thanks :-)), but a shorter “When performing a backup, all
> writes on the source subtree must go through the backup-top filter so it
> can copy all data to the target before it is changed.  Therefore,
> backup-top cannot allow other nodes to change data on its source child.”
> would work for me just as well.

Hmm, I don't like this "Therefore". For me the last statement
"cannot allow" doesn't looks like a consequence of the first
"all writes must go through", it more like rephrasing (still
not completely equal)... So, I'll keep my wording)

> 
>> So, trying to backup the configuration with writing side parents of
>> source sub-tree nodes should fail. Let's test it.
> Max
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

On 21.01.20 13:53, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 15:39, Max Reitz wrote:
>> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.01.2020 12:41, Max Reitz wrote:
>>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 21.01.2020 12:14, Max Reitz wrote:
>>>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>>>
>>>>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>>> ---
>>>>>>>>>      tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>>      tests/qemu-iotests/283.out |  8 ++++
>>>>>>>>>      tests/qemu-iotests/group   |  1 +
>>>>>>>>>      3 files changed, 84 insertions(+)
>>>>>>>>>      create mode 100644 tests/qemu-iotests/283
>>>>>>>>>      create mode 100644 tests/qemu-iotests/283.out
>>>>>>>>
>>>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>>>> more iotests during "make check-block"” series.
>>>>>>>>
>>>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000000..f0f216d109
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>>>> @@ -0,0 +1,75 @@
>>>>>>>>> +#!/usr/bin/env python
>>>>>>>>> +#
>>>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>>>> +#
>>>>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>>>>> +
>>>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>>>> +
>>>>>>>>> +size = 1024 * 1024
>>>>>>>>> +
>>>>>>>>> +"""
>>>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>>>
>>>>>>>> “The following configuration will become impossible”?
>>>>>>>
>>>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>>>>> is impossible with such configuration..
>>>>>>
>>>>>> But backup_top always unshares the write permission on the source.
>>>>>
>>>>> Yes, and I just try to say, that this action will fail. And the test checks that it
>>>>> fails (and it crashes with current master instead of fail).
>>>>
>>>> OK.  So what I was trying to say is that the comment currently only
>>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>>> correct that this fails (because all writes on the backup source must go
>>>> through backup_top), and that this is exactly what we want to test here.
>>>>
>>>> On first reading, I was wondering why exactly this comment would tell me
>>>> all these things, because I didn’t know what the test wants to test in
>>>> the first place.
>>>>
>>>> Max
>>>
>>> Hmm, something like:
>>>
>>> Backup wants to copy a point-in-time state of the source node. So, it catches all writes
>>> to the source node by appending backup-top filter above it. So we handle all changes which
>>> comes from source node parents. To prevent appearing of new writing parents during the
>>> progress, backup-top unshares write permission on its source child. This has additional
>>> implication: as this "unsharing" is propagated by default by backing/file children,
>>> backup-top conflicts with any side parents of source sub-tree with write permission.
>>> And this is in good relation with the general idea: with such parents we can't guarantee
>>> point-in-time backup.
>>
>> Works for me (thanks :-)), but a shorter “When performing a backup, all
>> writes on the source subtree must go through the backup-top filter so it
>> can copy all data to the target before it is changed.  Therefore,
>> backup-top cannot allow other nodes to change data on its source child.”
>> would work for me just as well.
> 
> Hmm, I don't like this "Therefore". For me the last statement
> "cannot allow" doesn't looks like a consequence of the first
> "all writes must go through", it more like rephrasing (still
> not completely equal)... So, I'll keep my wording)

I mean, you can just drop the second sentence, and then it gets even
shorter...

Max


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

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 15:39, Max Reitz wrote:
> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:41, Max Reitz wrote:
>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>>> 21.01.2020 12:14, Max Reitz wrote:
>>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>>
>>>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>> ---
>>>>>>>>      tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>      tests/qemu-iotests/283.out |  8 ++++
>>>>>>>>      tests/qemu-iotests/group   |  1 +
>>>>>>>>      3 files changed, 84 insertions(+)
>>>>>>>>      create mode 100644 tests/qemu-iotests/283
>>>>>>>>      create mode 100644 tests/qemu-iotests/283.out
>>>>>>>
>>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>>> more iotests during "make check-block"” series.
>>>>>>>
>>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..f0f216d109
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>>> @@ -0,0 +1,75 @@
>>>>>>>> +#!/usr/bin/env python
>>>>>>>> +#
>>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>>> +#
>>>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>>>> +
>>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>>> +
>>>>>>>> +size = 1024 * 1024
>>>>>>>> +
>>>>>>>> +"""
>>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>>
>>>>>>> “The following configuration will become impossible”?
>>>>>>
>>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>>>> is impossible with such configuration..
>>>>>
>>>>> But backup_top always unshares the write permission on the source.
>>>>
>>>> Yes, and I just try to say, that this action will fail. And the test checks that it
>>>> fails (and it crashes with current master instead of fail).
>>>
>>> OK.  So what I was trying to say is that the comment currently only
>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>> correct that this fails (because all writes on the backup source must go
>>> through backup_top), and that this is exactly what we want to test here.
>>>
>>> On first reading, I was wondering why exactly this comment would tell me
>>> all these things, because I didn’t know what the test wants to test in
>>> the first place.
>>>
>>> Max
>>
>> Hmm, something like:
>>
>> Backup wants to copy a point-in-time state of the source node. So, it catches all writes
>> to the source node by appending backup-top filter above it. So we handle all changes which
>> comes from source node parents. To prevent appearing of new writing parents during the
>> progress, backup-top unshares write permission on its source child. This has additional
>> implication: as this "unsharing" is propagated by default by backing/file children,
>> backup-top conflicts with any side parents of source sub-tree with write permission.
>> And this is in good relation with the general idea: with such parents we can't guarantee
>> point-in-time backup.
> 
> Works for me (thanks :-)), but a shorter “When performing a backup, all
> writes on the source subtree must go through the backup-top filter so it
> can copy all data to the target before it is changed.  Therefore,
> backup-top cannot allow other nodes to change data on its source child.”
> would work for me just as well.
> 
>> So, trying to backup the configuration with writing side parents of
>> source sub-tree nodes should fail. Let's test it.

But than, we need somehow link part about appending backup-top and so-on...

When performing a backup, all writes on the source subtree must go through the backup-top filter so it can copy all data to the target before it is changed.
backup-top filter is appended above source node, to achieve this thing, so all parents of source node are handled.
A configuration with side parents of source sub-tree with write permission is unsupported (we'd have append several backup-top filter like nodes to handle such parents).
The test create an example of such configuration and checks that backup fails.

-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

On 21.01.20 14:48, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 15:39, Max Reitz wrote:
>> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.01.2020 12:41, Max Reitz wrote:
>>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 21.01.2020 12:14, Max Reitz wrote:
>>>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>>>
>>>>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>>> ---
>>>>>>>>>      tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>>      tests/qemu-iotests/283.out |  8 ++++
>>>>>>>>>      tests/qemu-iotests/group   |  1 +
>>>>>>>>>      3 files changed, 84 insertions(+)
>>>>>>>>>      create mode 100644 tests/qemu-iotests/283
>>>>>>>>>      create mode 100644 tests/qemu-iotests/283.out
>>>>>>>>
>>>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>>>> more iotests during "make check-block"” series.
>>>>>>>>
>>>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000000..f0f216d109
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>>>> @@ -0,0 +1,75 @@
>>>>>>>>> +#!/usr/bin/env python
>>>>>>>>> +#
>>>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>>>> +#
>>>>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>>>>> +
>>>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>>>> +
>>>>>>>>> +size = 1024 * 1024
>>>>>>>>> +
>>>>>>>>> +"""
>>>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>>>
>>>>>>>> “The following configuration will become impossible”?
>>>>>>>
>>>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>>>>> is impossible with such configuration..
>>>>>>
>>>>>> But backup_top always unshares the write permission on the source.
>>>>>
>>>>> Yes, and I just try to say, that this action will fail. And the test checks that it
>>>>> fails (and it crashes with current master instead of fail).
>>>>
>>>> OK.  So what I was trying to say is that the comment currently only
>>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>>> correct that this fails (because all writes on the backup source must go
>>>> through backup_top), and that this is exactly what we want to test here.
>>>>
>>>> On first reading, I was wondering why exactly this comment would tell me
>>>> all these things, because I didn’t know what the test wants to test in
>>>> the first place.
>>>>
>>>> Max
>>>
>>> Hmm, something like:
>>>
>>> Backup wants to copy a point-in-time state of the source node. So, it catches all writes
>>> to the source node by appending backup-top filter above it. So we handle all changes which
>>> comes from source node parents. To prevent appearing of new writing parents during the
>>> progress, backup-top unshares write permission on its source child. This has additional
>>> implication: as this "unsharing" is propagated by default by backing/file children,
>>> backup-top conflicts with any side parents of source sub-tree with write permission.
>>> And this is in good relation with the general idea: with such parents we can't guarantee
>>> point-in-time backup.
>>
>> Works for me (thanks :-)), but a shorter “When performing a backup, all
>> writes on the source subtree must go through the backup-top filter so it
>> can copy all data to the target before it is changed.  Therefore,
>> backup-top cannot allow other nodes to change data on its source child.”
>> would work for me just as well.
>>
>>> So, trying to backup the configuration with writing side parents of
>>> source sub-tree nodes should fail. Let's test it.
> 
> But than, we need somehow link part about appending backup-top and so-on...
> 
> When performing a backup, all writes on the source subtree must go through the backup-top filter so it can copy all data to the target before it is changed.
> backup-top filter is appended above source node, to achieve this thing, so all parents of source node are handled.
> A configuration with side parents of source sub-tree with write permission is unsupported (we'd have append several backup-top filter like nodes to handle such parents).
> The test create an example of such configuration and checks that backup fails.

Sounds good!

(Except maybe s/that backup fails/that a backup is then not allowed/?
“backup fails” might also mean that the job just produces garbage.)

Max


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

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 16:51, Max Reitz wrote:
> On 21.01.20 14:48, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 15:39, Max Reitz wrote:
>>> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>>>> 21.01.2020 12:41, Max Reitz wrote:
>>>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 21.01.2020 12:14, Max Reitz wrote:
>>>>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>>>>
>>>>>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>>>> ---
>>>>>>>>>>       tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>       tests/qemu-iotests/283.out |  8 ++++
>>>>>>>>>>       tests/qemu-iotests/group   |  1 +
>>>>>>>>>>       3 files changed, 84 insertions(+)
>>>>>>>>>>       create mode 100644 tests/qemu-iotests/283
>>>>>>>>>>       create mode 100644 tests/qemu-iotests/283.out
>>>>>>>>>
>>>>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>>>>> more iotests during "make check-block"” series.
>>>>>>>>>
>>>>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 0000000000..f0f216d109
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>>>>> @@ -0,0 +1,75 @@
>>>>>>>>>> +#!/usr/bin/env python
>>>>>>>>>> +#
>>>>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>>>>> +#
>>>>>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>>>>>> +
>>>>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>>>>> +
>>>>>>>>>> +size = 1024 * 1024
>>>>>>>>>> +
>>>>>>>>>> +"""
>>>>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>>>>
>>>>>>>>> “The following configuration will become impossible”?
>>>>>>>>
>>>>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>>>>>> is impossible with such configuration..
>>>>>>>
>>>>>>> But backup_top always unshares the write permission on the source.
>>>>>>
>>>>>> Yes, and I just try to say, that this action will fail. And the test checks that it
>>>>>> fails (and it crashes with current master instead of fail).
>>>>>
>>>>> OK.  So what I was trying to say is that the comment currently only
>>>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>>>> correct that this fails (because all writes on the backup source must go
>>>>> through backup_top), and that this is exactly what we want to test here.
>>>>>
>>>>> On first reading, I was wondering why exactly this comment would tell me
>>>>> all these things, because I didn’t know what the test wants to test in
>>>>> the first place.
>>>>>
>>>>> Max
>>>>
>>>> Hmm, something like:
>>>>
>>>> Backup wants to copy a point-in-time state of the source node. So, it catches all writes
>>>> to the source node by appending backup-top filter above it. So we handle all changes which
>>>> comes from source node parents. To prevent appearing of new writing parents during the
>>>> progress, backup-top unshares write permission on its source child. This has additional
>>>> implication: as this "unsharing" is propagated by default by backing/file children,
>>>> backup-top conflicts with any side parents of source sub-tree with write permission.
>>>> And this is in good relation with the general idea: with such parents we can't guarantee
>>>> point-in-time backup.
>>>
>>> Works for me (thanks :-)), but a shorter “When performing a backup, all
>>> writes on the source subtree must go through the backup-top filter so it
>>> can copy all data to the target before it is changed.  Therefore,
>>> backup-top cannot allow other nodes to change data on its source child.”
>>> would work for me just as well.
>>>
>>>> So, trying to backup the configuration with writing side parents of
>>>> source sub-tree nodes should fail. Let's test it.
>>
>> But than, we need somehow link part about appending backup-top and so-on...
>>
>> When performing a backup, all writes on the source subtree must go through the backup-top filter so it can copy all data to the target before it is changed.
>> backup-top filter is appended above source node, to achieve this thing, so all parents of source node are handled.
>> A configuration with side parents of source sub-tree with write permission is unsupported (we'd have append several backup-top filter like nodes to handle such parents).
>> The test create an example of such configuration and checks that backup fails.
> 
> Sounds good!
> 
> (Except maybe s/that backup fails/that a backup is then not allowed/?
> “backup fails” might also mean that the job just produces garbage.)

OK for me. May be "backup is then not allowed (blockdev-backup command should fail)".

Should I resend? I think it's better drop "auto" mark and not create extra dependency on other series.


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

On 21.01.20 14:55, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 16:51, Max Reitz wrote:
>> On 21.01.20 14:48, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.01.2020 15:39, Max Reitz wrote:
>>>> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 21.01.2020 12:41, Max Reitz wrote:
>>>>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> 21.01.2020 12:14, Max Reitz wrote:
>>>>>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>>>>>
>>>>>>>>>>> Cc: qemu-stable@nongnu.org # v4.2.0
>>>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       tests/qemu-iotests/283     | 75 ++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>       tests/qemu-iotests/283.out |  8 ++++
>>>>>>>>>>>       tests/qemu-iotests/group   |  1 +
>>>>>>>>>>>       3 files changed, 84 insertions(+)
>>>>>>>>>>>       create mode 100644 tests/qemu-iotests/283
>>>>>>>>>>>       create mode 100644 tests/qemu-iotests/283.out
>>>>>>>>>>
>>>>>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>>>>>> more iotests during "make check-block"” series.
>>>>>>>>>>
>>>>>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 0000000000..f0f216d109
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>>>>>> @@ -0,0 +1,75 @@
>>>>>>>>>>> +#!/usr/bin/env python
>>>>>>>>>>> +#
>>>>>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>>>>>> +#
>>>>>>>>>>> +# Copyright (c) 2019 Virtuozzo 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 iotests
>>>>>>>>>>> +
>>>>>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
>>>>>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>>>>>> +
>>>>>>>>>>> +size = 1024 * 1024
>>>>>>>>>>> +
>>>>>>>>>>> +"""
>>>>>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>>>>>
>>>>>>>>>> “The following configuration will become impossible”?
>>>>>>>>>
>>>>>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write permission",
>>>>>>>>> is impossible with such configuration..
>>>>>>>>
>>>>>>>> But backup_top always unshares the write permission on the source.
>>>>>>>
>>>>>>> Yes, and I just try to say, that this action will fail. And the test checks that it
>>>>>>> fails (and it crashes with current master instead of fail).
>>>>>>
>>>>>> OK.  So what I was trying to say is that the comment currently only
>>>>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>>>>> correct that this fails (because all writes on the backup source must go
>>>>>> through backup_top), and that this is exactly what we want to test here.
>>>>>>
>>>>>> On first reading, I was wondering why exactly this comment would tell me
>>>>>> all these things, because I didn’t know what the test wants to test in
>>>>>> the first place.
>>>>>>
>>>>>> Max
>>>>>
>>>>> Hmm, something like:
>>>>>
>>>>> Backup wants to copy a point-in-time state of the source node. So, it catches all writes
>>>>> to the source node by appending backup-top filter above it. So we handle all changes which
>>>>> comes from source node parents. To prevent appearing of new writing parents during the
>>>>> progress, backup-top unshares write permission on its source child. This has additional
>>>>> implication: as this "unsharing" is propagated by default by backing/file children,
>>>>> backup-top conflicts with any side parents of source sub-tree with write permission.
>>>>> And this is in good relation with the general idea: with such parents we can't guarantee
>>>>> point-in-time backup.
>>>>
>>>> Works for me (thanks :-)), but a shorter “When performing a backup, all
>>>> writes on the source subtree must go through the backup-top filter so it
>>>> can copy all data to the target before it is changed.  Therefore,
>>>> backup-top cannot allow other nodes to change data on its source child.”
>>>> would work for me just as well.
>>>>
>>>>> So, trying to backup the configuration with writing side parents of
>>>>> source sub-tree nodes should fail. Let's test it.
>>>
>>> But than, we need somehow link part about appending backup-top and so-on...
>>>
>>> When performing a backup, all writes on the source subtree must go through the backup-top filter so it can copy all data to the target before it is changed.
>>> backup-top filter is appended above source node, to achieve this thing, so all parents of source node are handled.
>>> A configuration with side parents of source sub-tree with write permission is unsupported (we'd have append several backup-top filter like nodes to handle such parents).
>>> The test create an example of such configuration and checks that backup fails.
>>
>> Sounds good!
>>
>> (Except maybe s/that backup fails/that a backup is then not allowed/?
>> “backup fails” might also mean that the job just produces garbage.)
> 
> OK for me. May be "backup is then not allowed (blockdev-backup command should fail)".
> 
> Should I resend? I think it's better drop "auto" mark and not create extra dependency on other series.

I’d prefer a resend so I don’t modify the comment in a way you don’t want.

You can keep the test in auto, as I’ve just merged Thomas’s series
(which was the dependency).

Max


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