Re: [PATCH 29/29] iotests: Test block-export-* QMP interface

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8555 bytes --]

On 07.09.20 20:20, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py |  11 +++-
>  tests/qemu-iotests/307        | 117 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/307.out    | 111 ++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group      |   1 +
>  4 files changed, 239 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/307
>  create mode 100644 tests/qemu-iotests/307.out
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 6fed77487e..a842a9f92d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -67,7 +67,8 @@ if os.environ.get('QEMU_IO_OPTIONS_NO_FMT'):
>      qemu_io_args_no_fmt += \
>          os.environ['QEMU_IO_OPTIONS_NO_FMT'].strip().split(' ')
>  
> -qemu_nbd_args = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
> +qemu_nbd_prog = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]

It seems counterintuitive to me that something called “*_prog” would be
a list.

> +qemu_nbd_args = qemu_nbd_prog
>  if os.environ.get('QEMU_NBD_OPTIONS'):
>      qemu_nbd_args += os.environ['QEMU_NBD_OPTIONS'].strip().split(' ')
>  
> @@ -283,6 +284,14 @@ def qemu_nbd_early_pipe(*args: str) -> Tuple[int, str]:
>                                                     *full_args)
>      return returncode, output if returncode else ''
>  
> +def qemu_nbd_list(*args: str) -> str:
> +    '''Run qemu-nbd to list remote exports'''
> +    full_args = qemu_nbd_prog + ['-L'] + list(args)
> +    output, _ = qemu_tool_pipe_and_status('qemu-nbd', *full_args)
> +    log(output, filters=[filter_testfiles])

Not sure whether I like functions “silently” auto-logging the result.
I’d be happier if it were called *_log (i.e. qemu_nbd_list_log) like
most other functions that do.

> +    return output
> +
> +
>  @contextmanager
>  def qemu_nbd_popen(*args):
>      '''Context manager running qemu-nbd within the context'''

This would fit nicely into a dedicated patch.

*shrug*

> diff --git a/tests/qemu-iotests/307 b/tests/qemu-iotests/307
> new file mode 100755
> index 0000000000..06b9eeb15f
> --- /dev/null
> +++ b/tests/qemu-iotests/307
> @@ -0,0 +1,117 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (C) 2020 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: Kevin Wolf <kwolf@redhat.com>
> +#
> +# Test the block export QAPI interfaces
> +
> +import iotests
> +import os
> +
> +# Formats that have a request alignment != 1 result in different output for
> +# qemu-nbd -L ("min block")

So?  Why not filter that out?

I wonder anyway why the whole export output is logged.  It seems to me
like it would be sufficient to just log the number of exports available
and their names.

(Sure, logging everything gives us the advantage of perhaps finding NBD
bugs with this test even though it isn’t really an NBD test but intended
largely as a test for the new interface.  But when I see something like
“opt block” or “max block” in the output, I get a tingling sensation of
this potentially causing problems in the future depending on where this
test is running.)

((Which is funny, because now I notice I didn’t have this fear for “min
block”, but that’s where you actually did find an issue.))


Though all in all I don’t even know why it is that you write data at all
and use images, instead of just backing the export by a null-co node.

(I do admit I’m sending mixed signals here – first I ask for whether we
couldn’t make this test run on more formats, then I propose abandoning
physical images altogether.  I’m just emitting the questions that pop
into my head, and my head can be a confusing place from time to time.)

> +iotests.script_initialize(
> +    supported_fmts=['raw', 'qcow2', 'vmdk'],
> +    supported_platforms=['linux'],
> +)
> +
> +with iotests.FilePath('image') as img, \
> +     iotests.FilePath('socket') as socket, \

The second one should use base_dir=iotests.sock_dir.

> +     iotests.VM() as vm:
> +
> +    iotests.qemu_img('create', '-f', iotests.imgfmt, img, '64M')
> +    iotests.qemu_io_log('-f', iotests.imgfmt, '-c', 'write -P 0x11 0 4k', img)
> +
> +    iotests.log('=== Launch VM ===')
> +
> +    virtio_scsi_device = iotests.get_virtio_scsi_device()
> +
> +    vm.add_object('iothread,id=iothread0')
> +    vm.add_blockdev('file,filename=%s,node-name=file' % (img))
> +    vm.add_blockdev('%s,file=file,node-name=fmt' % (iotests.imgfmt))
> +    vm.add_blockdev('raw,file=file,node-name=ro,read-only=on')
> +    vm.add_device('id=scsi0,driver=%s,iothread=iothread0' % virtio_scsi_device)

Format strings would look nicer to me.  Perhaps not to you, though.

> +    vm.launch()
> +
> +    vm.qmp_log('nbd-server-start',
> +               addr={'type': 'unix', 'data': {'path': socket}},
> +               filters=(iotests.filter_qmp_testfiles, ))
> +    vm.qmp_log('query-block-exports')
> +
> +    iotests.log('\n=== Create a read-only NBD export ===')
> +
> +    vm.qmp_log('block-export-add', id='export0', type='nbd', node_name='fmt')
> +    vm.qmp_log('query-block-exports')
> +
> +    iotests.qemu_nbd_list('-k', socket)
> +
> +    iotests.log('\n=== Try a few invalid things ===')
> +
> +    vm.qmp_log('block-export-add', id='#invalid_id', type='nbd', node_name='fmt')
> +    vm.qmp_log('block-export-add', id='export0', type='nbd', node_name='fmt')
> +    vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='ro',
> +               writable=True)
> +    vm.qmp_log('block-export-del', id='export1')
> +    vm.qmp_log('query-block-exports')
> +
> +    iotests.log('\n=== Move export to an iothread ===')
> +
> +    vm.qmp_log('device_add', id='sda', driver='scsi-hd', drive='fmt')
> +    vm.qmp_log('query-block-exports')
> +    iotests.qemu_nbd_list('-k', socket)
> +
> +    iotests.log('\n=== Add a writable export ===')
> +
> +    # This fails because share-rw=off
> +    vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='fmt',
> +               name='export1', writable=True, writethrough=True,
> +               description='This is the writable second export')
> +
> +    vm.qmp_log('device_del', id='sda')
> +    event = vm.event_wait(name="DEVICE_DELETED",
> +                          match={'data': {'device': 'sda'}})
> +    iotests.log(event, filters=[iotests.filter_qmp_event])
> +    vm.qmp_log('device_add', id='sda', driver='scsi-hd', drive='fmt',
> +               share_rw=True)
> +
> +    # Now it should work
> +    vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='fmt',
> +               name='export1', writable=True, writethrough=True,
> +               description='This is the writable second export')
> +
> +    vm.qmp_log('query-block-exports')
> +    iotests.qemu_nbd_list('-k', socket)
> +
> +    iotests.log('\n=== Connect qemu-io to export1, try removing exports ===')
> +
> +    nbd_url = 'nbd+unix:///export1?socket=%s' % socket
> +    qemu_io = iotests.QemuIoInteractive('-f', 'raw', nbd_url)
> +
> +    iotests.log(qemu_io.cmd('read -P 0x11 0 4k'),
> +                filters=[iotests.filter_qemu_io])
> +    iotests.log(qemu_io.cmd('write -P 0x22 4k 4k'),
> +                filters=[iotests.filter_qemu_io])
> +
> +    vm.qmp_log('block-export-del', id='export1')
> +    vm.qmp_log('block-export-del', id='export0')

Should we check for the BLOCK_EXPORT_DELETED event?

> +    qemu_io.close()
> +
> +    vm.qmp_log('query-block-exports')
> +    iotests.qemu_nbd_list('-k', socket)

It might be nice to reconnect with qemu-io, and then see whether a hard
del works.

Max

> +
> +    iotests.log('\n=== Shut down QEMU ===')
> +    vm.shutdown()


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