On 21.01.20 13:53, 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. > > Hmm, I don't like this "Therefore". For me the last statement > "cannot allow" doesn't looks like a consequence of the first > "all writes must go through", it more like rephrasing (still > not completely equal)... So, I'll keep my wording)
I mean, you can just drop the second sentence, and then it gets even shorter... Max
signature.asc
Description: OpenPGP digital signature
