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

[Max Reitz]

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

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