Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature