Re: [Qemu-devel] [PATCH 08/14] qemu-iotests: Rewrite 206 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 206 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> All of the test cases stay the same as before, but in order to be able
> to implement proper job handling, the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  tests/qemu-iotests/206        | 705 
> +++++++++++++++++-------------------------
>  tests/qemu-iotests/206.out    | 241 +++++++++------
>  tests/qemu-iotests/group      |   2 +-
>  tests/qemu-iotests/iotests.py |  15 +
>  4 files changed, 448 insertions(+), 515 deletions(-)
> 
> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
> index 0a18b2b19a..9a305302d1 100755
> --- a/tests/qemu-iotests/206
> +++ b/tests/qemu-iotests/206

[...]

> +import iotests
> +from iotests import imgfmt
> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +def blockdev_create(vm, options):
> +    result = vm.qmp_log('x-blockdev-create', job_id='job0', options=options)
> +
> +    if 'return' in result:
> +        assert result['return'] == {}
> +        vm.run_job('job0')
> +    iotests.log("")
> +
> +with iotests.FilePath('t.qcow2') as disk_path, \
> +     iotests.FilePath('t.qcow2.base') as backing_path, \
> +     iotests.VM() as vm:
> +
> +    vm.add_object('secret,id=keysec0,data="foo"')

I don't know how subprocess.Popen() works, but are you sure you aren't
encrypting with '"foo"' now?  (i.e. literally that key, including quotes)

> +
> +    #
> +    # Successful image creation (defaults)
> +    #
> +    iotests.log("=== Successful image creation (defaults) ===")

OK, the comment makes sense for visual separation.  But so would leaving
three empty lines.  *cough*

> +    iotests.log("")
> +
> +    size = 128 * 1024 * 1024
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': 'file',
> +                          'filename': disk_path,
> +                          'size': 0 })
> +
> +    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> +               node_name='imgfile')
> +
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'imgfile',
> +                          'size': size })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)
> +

[...]

> +    #
> +    # Successful image creation (v2 non-default options)
> +    #
> +    iotests.log("=== Successful image creation (v2 non-default options) ===")
> +    iotests.log("")
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': 'file',
> +                          'filename': disk_path,
> +                          'size': 0 })
> +
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': {
> +                              'driver': 'file',
> +                              'filename': disk_path,
> +                          },
> +                          'size': size,
> +                          'backing-file': backing_path,

Change from the bash version: backing_path does not exist here.  Not
sure if that was intentional.

> +                          'backing-fmt': 'qcow2',
> +                          'version': 'v2',
> +                          'cluster-size': 512 })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)

[...]

> +    #
> +    # Invalid sizes
> +    #
> +    iotests.log("=== Invalid sizes ===")
> +
> +    # TODO Negative image sizes aren't handled correctly, but this is a 
> problem
> +    # with QAPI's implementation of the 'size' type and affects other 
> commands
> +    # as well. Once this is fixed, we may want to add a test case here.
> +    #
> +    # 1. Misaligned image size
> +    # 2. 2^64 - 512
> +    # 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
> +    # 4. 2^63 - 512 (generally valid, but qcow2 can't handle images this 
> size)
> +
> +    vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'node0',
> +                          'size': 1234  })
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'node0',
> +                          'size': 18446744073709551104 })
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'node0',
> +                          'size': 9223372036854775808 })
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'node0',
> +                          'size': 9223372036854775296})

Missing space before the closing fancy bracket, critical!

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 20ce5a0cf0..f0f4ef32f0 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -416,6 +416,21 @@ class VM(qtest.QEMUQtestMachine):
>          log(result)
>          return result
>  
> +    def run_job(self, job):

Is there any reason this function did not get its own patch?

Also, this is a function specifically for jobs that need a manual
dismiss but have auto-finalize.  That should be noted somewhere (ideally
in the function's name, but spontaneously I don't know how).

> +        while True:
> +            for ev in self.get_qmp_events_filtered(wait=True):
> +                if ev['event'] == 'JOB_STATUS_CHANGE':
> +                    if ev['data']['status'] == 'aborting':
> +                        result = self.qmp('query-jobs')
> +                        for j in result['return']:
> +                            log('Job failed: %s' % (j.get('error', None)))

I can understand that you didn't want to use just result['return'][0],
but if you do iterate, you should probably emit the job ID as well.

Max

> +                    elif ev['data']['status'] == 'concluded':
> +                        self.qmp_log('job-dismiss', id=job)
> +                    elif ev['data']['status'] == 'null':
> +                        return
> +                else:
> +                    iotests.log(ev)
> +
>  
>  index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')

signature.asc
Description: OpenPGP digital signature