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: qemu-sta...@nongnu.org # v4.2.0 >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>>>>> --- >>>>>> 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
