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

[Max Reitz]

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”?

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

signature.asc
Description: OpenPGP digital signature