Re: [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3920 bytes --]

On 2018-03-06 21:48, Stefan Hajnoczi wrote:
> This test case adds an NBD server export and then invokes
> blockdev-snapshot-sync, which changes the BlockDriverState node that the
> NBD server's BlockBackend points to.  This is an interesting scenario to
> test and exercises the code path fixed by the previous commit.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/208     | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/208.out |  9 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100755 tests/qemu-iotests/208
>  create mode 100644 tests/qemu-iotests/208.out
> 
> diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
> new file mode 100755
> index 0000000000..4e82b96c82
> --- /dev/null
> +++ b/tests/qemu-iotests/208
> @@ -0,0 +1,55 @@
> +#!/usr/bin/env python
> +#
> +# Copyright (C) 2018 Red Hat, Inc.
> +#
> +# 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/>.
> +#
> +# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>
> +#
> +# Check that the runtime NBD server does not crash when stopped after
> +# blockdev-snapshot-sync.
> +
> +import iotests
> +
> +with iotests.FilePath('disk.img') as disk_img_path, \
> +     iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
> +     iotests.FilePath('nbd.sock') as nbd_sock_path, \
> +     iotests.VM() as vm:
> +
> +    img_size = '10M'
> +    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk_img_path, img_size)
> +
> +    iotests.log('Launching VM...')
> +    (vm.add_drive(disk_img_path, 'node-name=drive0-node', interface='none')
> +       .launch())
> +
> +    iotests.log('Starting NBD server...')
> +    iotests.log(vm.qmp('nbd-server-start', addr={
> +            "type": "unix",
> +            "data": {
> +                "path": nbd_sock_path,
> +            }
> +        }))
> +
> +    iotests.log('Adding NBD export...')
> +    iotests.log(vm.qmp('nbd-server-add', device='drive0-node', writable=True))
> +
> +    iotests.log('Creating external snapshot...')
> +    iotests.log(vm.qmp('blockdev-snapshot-sync',
> +        node_name='drive0-node',
> +        snapshot_node_name='drive0-snapshot-node',
> +        snapshot_file=disk_snapshot_img_path))
> +
> +    iotests.log('Stopping NBD server...')
> +    iotests.log(vm.qmp('nbd-server-stop'))

Hm.  Tests what it's supposed to test.

However, I'm not sure whether that's what we want...  If I give a node
name to nbd-server-add, I'd probably expect the NBD server to stay at
that node.  We have BdrvChild.stay_at_node for that, so we could
implement it.  The question is whether we want to.

However, both wanting the snapshot to be below and above the NBD server
makes sense...  Perhaps it's best to leave it as-is, even if it is
surprising to some, because this way we at least don't have to switch to
RO mode (which we probably would if the NBD server is suddenly below the
snapshot).

I'm sure adding atomic graph change operations to QMP will fix all of
that.  I'm sure.

Anyway, since your test doesn't even check whether the NBD server is
above or below the snapshot (clever! :-))...

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]