Re: [PATCH] iotests: add test for removing persistent bitmap from backing file

[Max Reitz]

On 17.03.21 17:02, Vladimir Sementsov-Ogievskiy wrote:
> Just demonstrate one of x-blockdev-reopen usecases. We can't simply
> remove persistent bitmap from RO node (for example from backing file),
> as we need to remove it from the image too. So, we should reopen the
> node first.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   .../tests/remove-bitmap-from-backing          | 68 +++++++++++++++++++
>   .../tests/remove-bitmap-from-backing.out      |  6 ++
>   2 files changed, 74 insertions(+)
>   create mode 100755 tests/qemu-iotests/tests/remove-bitmap-from-backing
>   create mode 100644 tests/qemu-iotests/tests/remove-bitmap-from-backing.out
>
> diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing 
> b/tests/qemu-iotests/tests/remove-bitmap-from-backing
> new file mode 100755
> index 0000000000..121860d035
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
> @@ -0,0 +1,68 @@
> +#!/usr/bin/env python3
> +#
> +# Test removing persistent bitmap from backing
> +#
> +# Copyright (c) 2021 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
> +from iotests import log, qemu_img_create
> +
> +iotests.script_initialize(supported_fmts=['qcow2'])
> +
> +top, base = iotests.file_path('top', 'base')
> +size = '1M'
> +
> +qemu_img_create('-f', iotests.imgfmt, base, size)
> +qemu_img_create('-f', iotests.imgfmt, '-b', base, top, size)

I think qemu-img create nowadays complains when you use -b without -F. 
Also, I’d prefer an assert around this (i.e. assert qemu_img_create() == 0).

> +
> +iotests.qemu_img('bitmap', '--add', base ,'bitmap0')

s/ ,/, /

Which makes me notice that 297 doesn’t yet check tests/.  I’ll send a patch.

Also, again, an assert == 0 might be nice.

And then I wonder why you import qemu_img_create from iotests, but not 
qemu_img and qemu_img_pipe.

> +# Just assert that our method of checking bitmaps in the image works.
> +assert 'bitmaps' in iotests.qemu_img_pipe('info', base)
> +
> +vm = iotests.VM().add_drive(top, 'backing.node-name=base')
> +vm.launch()
> +
> +log('Trying to remove persistent bitmap from r-o base node, should fail:')
> +vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
> +
> +new_base_opts = {
> +    'node-name': 'base',
> +    'driver': 'qcow2',
> +    'file': {
> +        'driver': 'file',
> +        'filename':  base
> +    },
> +    'read-only': False
> +}
> +
> +# Don't want to bother with filtering qmp_log for reopen command
> +result = vm.qmp('x-blockdev-reopen', **new_base_opts)
> +if result != {'return': {}}:
> +    log('Failed to reopen: ' + str(result))
> +
> +log('Remove persistent bitmap from base node reopened to RW:')
> +vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
> +
> +new_base_opts['read-only'] = True
> +result = vm.qmp('x-blockdev-reopen', **new_base_opts)
> +if result != {'return': {}}:
> +    log('Failed to reopen: ' + str(result))
> +
> +vm.shutdown()
> +
> +if 'bitmaps' in iotests.qemu_img_pipe('info', base):
> +    "Hmm, bitmap is still in the base image. That's wrong"

With 297 covering tests/, it complains about this “pointless string 
statement”.  Shouldn’t this be something like

log('ERROR: Bitmap is still in the base image')

?

Apart from that more syntax-y stuff, the test looks good to me.

Max