Re: [Qemu-devel] [PATCH 38/42] job: Add JOB_STATUS_CHANGE QMP event

[Max Reitz]

On 2018-05-09 18:26, Kevin Wolf wrote:
> This adds a QMP event that is emitted whenever a job transitions from
> one status to another. For the event, a new qapi/job.json schema file is
> created which will contain all job-related definitions that aren't tied
> to the block layer.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  qapi/block-core.json       |  47 +-----------
>  qapi/job.json              |  65 +++++++++++++++++
>  qapi/qapi-schema.json      |   1 +
>  job.c                      |  10 +++
>  Makefile                   |   9 +++
>  Makefile.objs              |   4 +
>  tests/qemu-iotests/030     |   6 +-
>  tests/qemu-iotests/040     |   2 +
>  tests/qemu-iotests/041     |  17 ++++-
>  tests/qemu-iotests/095     |   2 +-
>  tests/qemu-iotests/095.out |   6 ++
>  tests/qemu-iotests/109     |   2 +-
>  tests/qemu-iotests/109.out | 178 
> +++++++++++++++++++++++++++++++++++++++------
>  tests/qemu-iotests/124     |   8 ++
>  tests/qemu-iotests/127.out |   7 ++
>  tests/qemu-iotests/141     |  10 +--
>  tests/qemu-iotests/141.out |  29 ++++++++
>  tests/qemu-iotests/144     |   2 +-
>  tests/qemu-iotests/144.out |   7 ++
>  tests/qemu-iotests/156     |   2 +-
>  tests/qemu-iotests/156.out |   7 ++
>  tests/qemu-iotests/185     |  12 +--
>  tests/qemu-iotests/185.out |  10 +++
>  tests/qemu-iotests/191     |   4 +-
>  tests/qemu-iotests/191.out | 132 +++++++++++++++++++++++++++++++++
>  25 files changed, 492 insertions(+), 87 deletions(-)
>  create mode 100644 qapi/job.json

Your effort to keep this series in 42 patches in Ehren, but I think it
would make sense to have a separate patch that creates qapi/job.json. ;-)

[...]

> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 640a6dfd10..03aea460c9 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -304,7 +304,7 @@ class TestParallelOps(iotests.QMPTestCase):
>          result = self.vm.qmp('block-stream', device='node5', 
> base=self.imgs[3], job_id='stream-node6')
>          self.assert_qmp(result, 'error/class', 'GenericError')
>  
> -        event = self.vm.get_qmp_event(wait=True)
> +        event = self.vm.event_wait(name='BLOCK_JOB_READY')
>          self.assertEqual(event['event'], 'BLOCK_JOB_READY')

This assertion is a bit useless, now, but whatever.

>          self.assert_qmp(event, 'data/device', 'commit-drive0')
>          self.assert_qmp(event, 'data/type', 'commit')
> @@ -751,7 +751,9 @@ class TestStreamStop(iotests.QMPTestCase):
>  
>          time.sleep(0.1)
>          events = self.vm.get_qmp_events(wait=False)
> -        self.assertEqual(events, [], 'unexpected QMP event: %s' % events)
> +        for e in events:
> +            if e['event'] != 'JOB_STATUS_CHANGE':
> +                self.assertEqual(events, [], 'unexpected QMP event: %s' % 
> events)

self.fail() would be nicer to read.

>  
>          self.cancel_and_wait(resume=True)
>  

[...]

> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index a860a31e9a..e94587950c 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -445,6 +445,8 @@ new_state = "1"
>                      self.assert_qmp(event, 'data/device', 'drive0')
>                      self.assert_qmp(event, 'data/error', 'Input/output 
> error')
>                      completed = True
> +                elif event['event'] == 'JOB_STATUS_CHANGE':
> +                    self.assert_qmp(event, 'data/id', 'drive0')

There were plenty of such loops in 030.  Why did you leave them as they
are and add this check here?

(I can understand it in the previous hunk, because that one has an else
branch, but this one does not.)

((And if you decide to always do this check, iotests.py needs it, too.))

>  
>          self.assert_no_active_block_jobs()
>          self.vm.shutdown()
> @@ -457,6 +459,10 @@ new_state = "1"
>          self.assert_qmp(result, 'return', {})
>  
>          event = self.vm.get_qmp_event(wait=True)
> +        while event['event'] == 'JOB_STATUS_CHANGE':
> +            self.assert_qmp(event, 'data/id', 'drive0')
> +            event = self.vm.get_qmp_event(wait=True)
> +
>          self.assertEquals(event['event'], 'BLOCK_JOB_ERROR')

Wouldn't this be simpler with
event = self.vm.event_wait(name='BLOCK_JOB_ERROR')?

Same in other hunks in this file.

(And technically in all cases (like 056) that use event_wait already.)

(Sure, if you want to check @id...  But then you'd have to do the same
in 030.  And I don't quite see the point of checking JOB_STATUS_CHANGE
just sporadically, and not even checking the target status.)

>          self.assert_qmp(event, 'data/device', 'drive0')
>          self.assert_qmp(event, 'data/operation', 'read')

[...]

> diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
> index 2f9d7b9bc2..9ae23a6c63 100755
> --- a/tests/qemu-iotests/141
> +++ b/tests/qemu-iotests/141

[...]

> @@ -179,7 +179,7 @@ test_blockjob \
>                      'device': 'drv0',
>                      'speed': 1}}" \
>      'return' \
> -    'BLOCK_JOB_CANCELLED'
> +    '"status": "null"'
>  

The comment above this hunk says that "no event other than
BLOCK_JOB_CANCELLED will be emitted".  It would make sense to change it
to e.g. "no block job event other than [...]".

>  _cleanup_qemu
>  

[...]

You forgot to adjust 094, and although I cannot prove it (I don't have
an nfs setup handy right now), I have a hunch that this patch breaks 173
as well.

Max

signature.asc
Description: OpenPGP digital signature