If the timeout is 0, we can get None back. Handle this explicitly.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 2f12cebde40..a835b7550da 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -32,7 +32,7 @@
 from types import TracebackType
 
 from . import qmp
-from .qmp import SocketAddrT
+from .qmp import SocketAddrT, QMPMessage
 
 LOG = logging.getLogger(__name__)
 
@@ -553,6 +553,8 @@ def _match(event):
                     return True
             return False
 
+        event: Optional[QMPMessage]
+
         # Search cached events
         for event in self._events:
             if _match(event):
@@ -562,6 +564,8 @@ def _match(event):
         # Poll for new events
         while True:
             event = self._qmp.pull_event(wait=timeout)
+            if event is None:
+                break
             if _match(event):
                 return event
             self._events.append(event)
-- 
2.21.3

Am 02.06.2020 um 23:45 hat John Snow geschrieben:
> If the timeout is 0, we can get None back. Handle this explicitly.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Subject line: This is events_wait(), not event_wait(). Both functions
exist.

> @@ -562,6 +564,8 @@ def _match(event):
>          # Poll for new events
>          while True:
>              event = self._qmp.pull_event(wait=timeout)
> +            if event is None:
> +                break
>              if _match(event):
>                  return event
>              self._events.append(event)

Hm... How could this ever work? I guess we just never really tested
whether timeouts actually time out?

(It's still somewhat unintuitive that receiving an unrelated event
resets the timeout, but not the problem of this series...)

Kevin

On 6/4/20 10:20 AM, Kevin Wolf wrote:
> Am 02.06.2020 um 23:45 hat John Snow geschrieben:
>> If the timeout is 0, we can get None back. Handle this explicitly.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Subject line: This is events_wait(), not event_wait(). Both functions
> exist.
> 
>> @@ -562,6 +564,8 @@ def _match(event):
>>          # Poll for new events
>>          while True:
>>              event = self._qmp.pull_event(wait=timeout)
>> +            if event is None:
>> +                break
>>              if _match(event):
>>                  return event
>>              self._events.append(event)
> 
> Hm... How could this ever work? I guess we just never really tested
> whether timeouts actually time out?
> 

It's weirder than you think.

pull_event, when a timeout is supplied, will pass that timeout to
QEMUMonitorProtocol.__get_events, which populates the received event
queue but does not return data on the stack. If there are no events in
the queue and we are given a timeout, we will raise QMPTimeoutError if
no event appears in that timeframe.

pull_event() will only return None in the case that you don't give it a
timeout or tell it to wait.

The typing of events_wait allows us to specify a timeout of 0 here,
which is treated as no-wait down the stack, which may return None if
there was no such event ready.

Thus, break and also return None.

(I should update the docstring here.)

> (It's still somewhat unintuitive that receiving an unrelated event
> resets the timeout, but not the problem of this series...)
> 
> Kevin
> 

Yes, it's weird. It's not a timeout on this one event, it's a timeout on
event-receiving activity. In practice, it works quite well for the
iotest suite where there are often not other event drivers present. It
was a quick hack (that I introduced!) to ensure that iotests would halt
in some finite amount of time.

--js