[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 08/14] qemu-iotests: Rewrite 206 for blockdev-cr
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH 08/14] qemu-iotests: Rewrite 206 for blockdev-create job |
Date: |
Tue, 29 May 2018 20:49:04 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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 <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)
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
signature.asc
Description: PGP signature
- Re: [Qemu-block] [PATCH 07/14] qemu-iotests: Add iotests.img_info_log(), (continued)
- [Qemu-block] [PATCH 09/14] qemu-iotests: Rewrite 207 for blockdev-create job, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 10/14] qemu-iotests: Rewrite 210 for blockdev-create job, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 08/14] qemu-iotests: Rewrite 206 for blockdev-create job, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 11/14] qemu-iotests: Rewrite 211 for blockdev-create job, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 12/14] qemu-iotests: Rewrite 212 for blockdev-create job, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 13/14] qemu-iotests: Rewrite 213 for blockdev-create job, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 14/14] block/create: Mark blockdev-create stable, Kevin Wolf, 2018/05/25
- Re: [Qemu-block] [Qemu-devel] [PATCH 00/14] block: Make blockdev-create a job and stable API, no-reply, 2018/05/25