[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] iotests: add test for backup-top failure on permission a
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation |
Date: |
Tue, 21 Jan 2020 09:23:06 +0000 |
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).
>
>>> 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
- [PATCH 0/2] backup-top failure path fix, Vladimir Sementsov-Ogievskiy, 2020/01/16
- [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/16
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/20
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/20
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation,
Vladimir Sementsov-Ogievskiy <=
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
[PATCH 1/2] block/backup-top: fix failure path, Vladimir Sementsov-Ogievskiy, 2020/01/16