| Message ID | 20200116155452.30972-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | backup-top failure path fix | expand |
On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote: > This test checks that bug is really fixed by previous commit. > > Cc: qemu-stable@nongnu.org # v4.2.0 > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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”? 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. :-) > + > +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. 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
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-stable@nongnu.org # v4.2.0 >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.. > > 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/ > >> + >> +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 >
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-stable@nongnu.org # v4.2.0 >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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. >> 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 >> > >
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-stable@nongnu.org # v4.2.0 >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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 >>> >> >> > >
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-stable@nongnu.org # v4.2.0 >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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 >>>> 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 >>>> >>> >>> >> >> > >
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-stable@nongnu.org # v4.2.0 >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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. So, trying to backup the configuration with writing side parents of source sub-tree nodes should fail. Let's test it. > >>>>> 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 >>>>> >>>> >>>> >>> >>> >> >> > >
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-stable@nongnu.org # v4.2.0 >>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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. Max
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-stable@nongnu.org # v4.2.0 >>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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) > >> So, trying to backup the configuration with writing side parents of >> source sub-tree nodes should fail. Let's test it. > Max >
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-stable@nongnu.org # v4.2.0 >>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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
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-stable@nongnu.org # v4.2.0 >>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
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-stable@nongnu.org # v4.2.0 >>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.) Max
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-stable@nongnu.org # v4.2.0 >>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
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: qemu-stable@nongnu.org # v4.2.0 >>>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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. 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
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: + + ┌────────┐ target ┌─────────────┐ + │ target │ ◀─────── │ backup_top │ + └────────┘ └─────────────┘ + │ + │ backing + ▼ + ┌─────────────┐ + │ source │ + └─────────────┘ + │ + │ file + ▼ + ┌─────────────┐ write perm ┌───────┐ + │ base │ ◀──────────── │ other │ + └─────────────┘ └───────┘ + +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/283.out b/tests/qemu-iotests/283.out new file mode 100644 index 0000000000..daaf5828c1 --- /dev/null +++ b/tests/qemu-iotests/283.out @@ -0,0 +1,8 @@ +{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}} +{"return": {}} +{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}} +{"return": {}} +{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} +{"return": {}} +{"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} +{"error": {"class": "GenericError", "desc": "Cannot set permissions for backup-top filter: Conflicts with use by other as 'image', which uses 'write' on base"}} 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
This test checks that bug is really fixed by previous commit. Cc: qemu-stable@nongnu.org # v4.2.0 Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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