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

[Max Reitz]

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.

>> 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
>>
> 
>

signature.asc
Description: OpenPGP digital signature