| Message ID | 20190523170643.20794-4-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | blockdev-backup: don't check aio_context too early | expand |
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.)
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
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
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 --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)
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(-)