21.01.2020 16:51, Max Reitz wrote: > On 21.01.20 14:48, Vladimir Sementsov-Ogievskiy wrote: >> 21.01.2020 15:39, Max Reitz wrote: >>> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote: >>>> 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. >>> >>> Works for me (thanks :-)), but a shorter “When performing a backup, all >>> writes on the source subtree must go through the backup-top filter so it >>> can copy all data to the target before it is changed. Therefore, >>> backup-top cannot allow other nodes to change data on its source child.” >>> would work for me just as well. >>> >>>> So, trying to backup the configuration with writing side parents of >>>> source sub-tree nodes should fail. Let's test it. >> >> But than, we need somehow link part about appending backup-top and so-on... >> >> When performing a backup, all writes on the source subtree must go through >> the backup-top filter so it can copy all data to the target before it is >> changed. >> backup-top filter is appended above source node, to achieve this thing, so >> all parents of source node are handled. >> A configuration with side parents of source sub-tree with write permission >> is unsupported (we'd have append several backup-top filter like nodes to >> handle such parents). >> The test create an example of such configuration and checks that backup >> fails. > > Sounds good! > > (Except maybe s/that backup fails/that a backup is then not allowed/? > “backup fails” might also mean that the job just produces garbage.)
OK for me. May be "backup is then not allowed (blockdev-backup command should fail)". Should I resend? I think it's better drop "auto" mark and not create extra dependency on other series. -- Best regards, Vladimir
