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