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: address@hidden # v4.2.0
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>>> ---
>>>>>>     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