Re: [PATCH v4 1/3] qmp.py: change event_wait to use a dict

[Kevin Wolf <kwolf@redhat.com>]

Am 14.05.2020 um 04:25 hat John Snow geschrieben:
> It's easier to work with than a list of tuples, because we can check the
> keys for membership.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine.py        | 10 +++++-----
>  tests/qemu-iotests/040        | 12 ++++++------
>  tests/qemu-iotests/260        |  5 +++--
>  tests/qemu-iotests/iotests.py | 16 ++++++++--------
>  4 files changed, 22 insertions(+), 21 deletions(-)

I think you need to convert scripts/simplebench/bench_block_job.py, too.

> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index b9a98e2c86..eaedc25172 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -478,21 +478,21 @@ def event_wait(self, name, timeout=60.0, match=None):
>          timeout: QEMUMonitorProtocol.pull_event timeout parameter.
>          match: Optional match criteria. See event_match for details.
>          """
> -        return self.events_wait([(name, match)], timeout)
> +        return self.events_wait({name: match}, timeout)
>  
>      def events_wait(self, events, timeout=60.0):
>          """
>          events_wait waits for and returns a named event from QMP with a timeout.
>  
> -        events: a sequence of (name, match_criteria) tuples.
> +        events: a mapping containing {name: match_criteria}.
>                  The match criteria are optional and may be None.
>                  See event_match for details.  timeout:
>                  QEMUMonitorProtocol.pull_event timeout parameter.
>                  """
>          def _match(event):
> -            for name, match in events:
> -                if event['event'] == name and self.event_match(event, match):
> -                    return True
> +            name = event['event']
> +            if name in events:
> +                return self.event_match(event, events[name])

This part confused me a bit for a second. Of course, that's probably
mostly just me, but I feel 'events' isn't a good name any more when the
values of the dict are match strings rather than events.

>              return False
>  
>          # Search cached events
> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index 32c82b4ec6..90b59081ff 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase):
>  
>      def run_job(self, expected_events, error_pauses_job=False):
>          match_device = {'data': {'device': 'job0'}}
> -        events = [
> -            ('BLOCK_JOB_COMPLETED', match_device),
> -            ('BLOCK_JOB_CANCELLED', match_device),
> -            ('BLOCK_JOB_ERROR', match_device),
> -            ('BLOCK_JOB_READY', match_device),
> -        ]
> +        events = {
> +            'BLOCK_JOB_COMPLETED': match_device,
> +            'BLOCK_JOB_CANCELLED': match_device,
> +            'BLOCK_JOB_ERROR': match_device,
> +            'BLOCK_JOB_READY': match_device,
> +        }
>  
>          completed = False
>          log = []
> diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
> index 804a7addb9..729f031122 100755
> --- a/tests/qemu-iotests/260
> +++ b/tests/qemu-iotests/260
> @@ -67,8 +67,9 @@ def test(persistent, restart):
>  
>      vm.qmp_log('block-commit', device='drive0', top=top,
>                 filters=[iotests.filter_qmp_testfiles])
> -    ev = vm.events_wait((('BLOCK_JOB_READY', None),
> -                         ('BLOCK_JOB_COMPLETED', None)))
> +    ev = vm.events_wait({
> +        'BLOCK_JOB_READY': None,
> +        'BLOCK_JOB_COMPLETED': None })

So, I'm not sure if this is nitpicking or rather bikeshedding, but
having the closing brackets on the next line would be more consistent
with the other instances in this patch.

>      log(filter_qmp_event(ev))
>      if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
>          vm.shutdown()

Kevin