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

[Kevin Wolf <kwolf@redhat.com>]

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

Am 29.05.2018 um 14:27 hat Max Reitz geschrieben:
> 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 <kwolf@redhat.com>
> > ---
> >  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)

That's correct. Anything wrong with it?

(Okay, you're right. I did fix it in 210, but forgot it here...)

> > +
> > +    #
> > +    # 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*

I tried vertical spacing first, but it didn't work well for me. What
made the difference is the syntax highlighting of comments vs. code.
YMMV.

> > +    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.

No, it was not. It's actually a good test case, though. Creating an
image at backing_path would be easy enough, but maybe we should just
leave it?

> > +                          '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).

If you want me to move it into a separate patch, maybe just make it more
reusable with parameters auto_finalize=True, auto_dismiss=false.

> > +        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.

If you have multiple jobs running, it doesn't work correctly anyway. :-)

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]