diff mbox series

[v3,3/5] QEMUMachine: add events_wait method

Message ID 20190523170643.20794-4-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series blockdev-backup: don't check aio_context too early | expand

Commit Message

John Snow May 23, 2019, 5:06 p.m. UTC
Instead of event_wait which looks for a single event, add an events_wait
which can look for any number of events simultaneously. However, it
will still only return one at a time, whichever happens first.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 20 deletions(-)

Comments

Max Reitz May 23, 2019, 5:49 p.m. UTC | #1
On 23.05.19 19:06, John Snow wrote:
> Instead of event_wait which looks for a single event, add an events_wait
> which can look for any number of events simultaneously. However, it
> will still only return one at a time, whichever happens first.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> index 81d9657ec0..98ed8a2e28 100644
> --- a/python/qemu/__init__.py
> +++ b/python/qemu/__init__.py
> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>          self._qmp.clear_events()
>          return events
>  
> -    def event_wait(self, name, timeout=60.0, match=None):
> +    @staticmethod
> +    def event_match(event, match=None):
>          """
> -        Wait for specified timeout on named event in QMP; optionally filter
> -        results by match.
> +        Check if an event matches optional match criteria.
>  
> -        The 'match' is checked to be a recursive subset of the 'event'; skips
> -        branch processing on match's value None
> -           {"foo": {"bar": 1}} matches {"foo": None}
> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
> +        The match criteria takes the form of a matching subdict. The event is
> +        checked to be a superset of the subdict, recursively, with matching
> +        values whenever those values are not None.
> +
> +        Examples, with the subdict queries on the left:
> +         - None matches any object.
> +         - {"foo": None} matches {"foo": {"bar": 1}}
> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}

Pre-existing, but the difference between “bar” and “baz” confused me
quite a bit.

Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
Does that make sense?  Shouldn’t None be the wildcard here in general?
(Also pre-existing of course.)

But this patch doesn’t make things worse, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

(I’d still like your opinion.)
John Snow May 23, 2019, 6:03 p.m. UTC | #2
On 5/23/19 1:49 PM, Max Reitz wrote:
> On 23.05.19 19:06, John Snow wrote:
>> Instead of event_wait which looks for a single event, add an events_wait
>> which can look for any number of events simultaneously. However, it
>> will still only return one at a time, whichever happens first.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>> index 81d9657ec0..98ed8a2e28 100644
>> --- a/python/qemu/__init__.py
>> +++ b/python/qemu/__init__.py
>> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>>          self._qmp.clear_events()
>>          return events
>>  
>> -    def event_wait(self, name, timeout=60.0, match=None):
>> +    @staticmethod
>> +    def event_match(event, match=None):
>>          """
>> -        Wait for specified timeout on named event in QMP; optionally filter
>> -        results by match.
>> +        Check if an event matches optional match criteria.
>>  
>> -        The 'match' is checked to be a recursive subset of the 'event'; skips
>> -        branch processing on match's value None
>> -           {"foo": {"bar": 1}} matches {"foo": None}
>> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>> +        The match criteria takes the form of a matching subdict. The event is
>> +        checked to be a superset of the subdict, recursively, with matching
>> +        values whenever those values are not None.
>> +
>> +        Examples, with the subdict queries on the left:
>> +         - None matches any object.
>> +         - {"foo": None} matches {"foo": {"bar": 1}}
>> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
> 
> Pre-existing, but the difference between “bar” and “baz” confused me
> quite a bit.
> 
> Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
> Does that make sense?  Shouldn’t None be the wildcard here in general?
> (Also pre-existing of course.)
> 
> But this patch doesn’t make things worse, so:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> (I’d still like your opinion.)
> 

I knew I was inviting trouble by trying to re-document this.

The intention I had when writing the docs, which I think are wrong now,
was for {"foo": None} to match {"foo": 1}, but I think you're right that
it won't because '1' isn't a dict, so it tests for equality instead.

So I need to fix this one up a little bit, but I'll take the review as a
sign that this approach seems workable.

--js
Max Reitz May 24, 2019, 12:38 p.m. UTC | #3
On 23.05.19 20:03, John Snow wrote:
> 
> 
> On 5/23/19 1:49 PM, Max Reitz wrote:
>> On 23.05.19 19:06, John Snow wrote:
>>> Instead of event_wait which looks for a single event, add an events_wait
>>> which can look for any number of events simultaneously. However, it
>>> will still only return one at a time, whichever happens first.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>>> index 81d9657ec0..98ed8a2e28 100644
>>> --- a/python/qemu/__init__.py
>>> +++ b/python/qemu/__init__.py
>>> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>>>          self._qmp.clear_events()
>>>          return events
>>>  
>>> -    def event_wait(self, name, timeout=60.0, match=None):
>>> +    @staticmethod
>>> +    def event_match(event, match=None):
>>>          """
>>> -        Wait for specified timeout on named event in QMP; optionally filter
>>> -        results by match.
>>> +        Check if an event matches optional match criteria.
>>>  
>>> -        The 'match' is checked to be a recursive subset of the 'event'; skips
>>> -        branch processing on match's value None
>>> -           {"foo": {"bar": 1}} matches {"foo": None}
>>> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>>> +        The match criteria takes the form of a matching subdict. The event is
>>> +        checked to be a superset of the subdict, recursively, with matching
>>> +        values whenever those values are not None.
>>> +
>>> +        Examples, with the subdict queries on the left:
>>> +         - None matches any object.
>>> +         - {"foo": None} matches {"foo": {"bar": 1}}
>>> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
>>
>> Pre-existing, but the difference between “bar” and “baz” confused me
>> quite a bit.
>>
>> Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
>> Does that make sense?  Shouldn’t None be the wildcard here in general?
>> (Also pre-existing of course.)
>>
>> But this patch doesn’t make things worse, so:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> (I’d still like your opinion.)
>>
> 
> I knew I was inviting trouble by trying to re-document this.
> 
> The intention I had when writing the docs, which I think are wrong now,
> was for {"foo": None} to match {"foo": 1}, but I think you're right that
> it won't because '1' isn't a dict, so it tests for equality instead.
> 
> So I need to fix this one up a little bit, but I'll take the review as a
> sign that this approach seems workable.

I think the comment is technically completely correct.  It’s just that
(1) it’s hard to discern “bar” from “baz”, and (2) if {"foo": None}
intentionally does not match {"foo": 1}, we may want to document that,
because it isn’t intuitively clear from the description.  If it’s a bug,
the code should be fixed (and it should still be documented).

Max
John Snow May 24, 2019, 5:57 p.m. UTC | #4
On 5/24/19 8:38 AM, Max Reitz wrote:
> On 23.05.19 20:03, John Snow wrote:
>>
>>
>> On 5/23/19 1:49 PM, Max Reitz wrote:
>>> On 23.05.19 19:06, John Snow wrote:
>>>> Instead of event_wait which looks for a single event, add an events_wait
>>>> which can look for any number of events simultaneously. However, it
>>>> will still only return one at a time, whichever happens first.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>>>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>>>> index 81d9657ec0..98ed8a2e28 100644
>>>> --- a/python/qemu/__init__.py
>>>> +++ b/python/qemu/__init__.py
>>>> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>>>>          self._qmp.clear_events()
>>>>          return events
>>>>  
>>>> -    def event_wait(self, name, timeout=60.0, match=None):
>>>> +    @staticmethod
>>>> +    def event_match(event, match=None):
>>>>          """
>>>> -        Wait for specified timeout on named event in QMP; optionally filter
>>>> -        results by match.
>>>> +        Check if an event matches optional match criteria.
>>>>  
>>>> -        The 'match' is checked to be a recursive subset of the 'event'; skips
>>>> -        branch processing on match's value None
>>>> -           {"foo": {"bar": 1}} matches {"foo": None}
>>>> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>>>> +        The match criteria takes the form of a matching subdict. The event is
>>>> +        checked to be a superset of the subdict, recursively, with matching
>>>> +        values whenever those values are not None.
>>>> +
>>>> +        Examples, with the subdict queries on the left:
>>>> +         - None matches any object.
>>>> +         - {"foo": None} matches {"foo": {"bar": 1}}
>>>> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
>>>
>>> Pre-existing, but the difference between “bar” and “baz” confused me
>>> quite a bit.
>>>
>>> Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
>>> Does that make sense?  Shouldn’t None be the wildcard here in general?
>>> (Also pre-existing of course.)
>>>
>>> But this patch doesn’t make things worse, so:
>>>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>
>>> (I’d still like your opinion.)
>>>
>>
>> I knew I was inviting trouble by trying to re-document this.
>>
>> The intention I had when writing the docs, which I think are wrong now,
>> was for {"foo": None} to match {"foo": 1}, but I think you're right that
>> it won't because '1' isn't a dict, so it tests for equality instead.
>>
>> So I need to fix this one up a little bit, but I'll take the review as a
>> sign that this approach seems workable.
> 
> I think the comment is technically completely correct.  It’s just that
> (1) it’s hard to discern “bar” from “baz”, and (2) if {"foo": None}
> intentionally does not match {"foo": 1}, we may want to document that,
> because it isn’t intuitively clear from the description.  If it’s a bug,
> the code should be fixed (and it should still be documented).
> 
> Max
> 

Yeah, I see. OK; I have prepared a patch that can be applied
independently after this series, if you'd like to stage this as-is. The
new patch changes behavior of this function a little, but is backwards
compatible with no changes because nobody was using these edge cases.

--js
diff mbox series

Patch

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 81d9657ec0..98ed8a2e28 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -402,42 +402,71 @@  class QEMUMachine(object):
         self._qmp.clear_events()
         return events
 
-    def event_wait(self, name, timeout=60.0, match=None):
+    @staticmethod
+    def event_match(event, match=None):
         """
-        Wait for specified timeout on named event in QMP; optionally filter
-        results by match.
+        Check if an event matches optional match criteria.
 
-        The 'match' is checked to be a recursive subset of the 'event'; skips
-        branch processing on match's value None
-           {"foo": {"bar": 1}} matches {"foo": None}
-           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
+        The match criteria takes the form of a matching subdict. The event is
+        checked to be a superset of the subdict, recursively, with matching
+        values whenever those values are not None.
+
+        Examples, with the subdict queries on the left:
+         - None matches any object.
+         - {"foo": None} matches {"foo": {"bar": 1}}
+         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
+         - {"foo": {"baz": 2}} matches {"foo": {"bar": 1, "baz": 2}}
         """
-        def event_match(event, match=None):
-            if match is None:
-                return True
+        if match is None:
+            return True
 
-            for key in match:
-                if key in event:
-                    if isinstance(event[key], dict):
-                        if not event_match(event[key], match[key]):
-                            return False
-                    elif event[key] != match[key]:
+        for key in match:
+            if key in event:
+                if isinstance(event[key], dict):
+                    if not QEMUMachine.event_match(event[key], match[key]):
                         return False
-                else:
+                elif event[key] != match[key]:
                     return False
+            else:
+                return False
+        return True
 
-            return True
+    def event_wait(self, name, timeout=60.0, match=None):
+        """
+        event_wait waits for and returns a named event from QMP with a timeout.
+
+        name: The event to wait for.
+        timeout: QEMUMonitorProtocol.pull_event timeout parameter.
+        match: Optional match criteria. See event_match for details.
+        """
+        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.
+                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
+            return False
 
         # Search cached events
         for event in self._events:
-            if (event['event'] == name) and event_match(event, match):
+            if _match(event):
                 self._events.remove(event)
                 return event
 
         # Poll for new events
         while True:
             event = self._qmp.pull_event(wait=timeout)
-            if (event['event'] == name) and event_match(event, match):
+            if _match(event):
                 return event
             self._events.append(event)