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

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