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

[Max Reitz]

On 21.01.20 14:55, Vladimir Sementsov-Ogievskiy wrote:
> 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: 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.
>>>
>>> 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.

I’d prefer a resend so I don’t modify the comment in a way you don’t want.

You can keep the test in auto, as I’ve just merged Thomas’s series
(which was the dependency).

Max

signature.asc
Description: OpenPGP digital signature