| Message ID | 20201006235817.3280413-8-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python/qemu: strictly typed mypy conversion, pt2 | expand |
Am 07.10.2020 um 01:58 hat John Snow geschrieben: > Like many other Optional[] types, it's not always a given that this > object will be set. Wrap it in a type-shim that raises a meaningful > error and will always return a concrete type. > > Signed-off-by: John Snow <jsnow@redhat.com> > @@ -515,11 +515,13 @@ def set_qmp_monitor(self, enabled=True): > line. Default is True. > @note: call this function before launch(). > """ > - if enabled: > - self._qmp_set = True > - else: > - self._qmp_set = False > - self._qmp = None > + self._qmp_set = enabled This change seems unrelated to wrapping the connection in a property. Intuitively, it makes sense that the connection of a running instance doesn't go away just because I disable QMP in the command line for the next launch. If this is the reasoning behind the change, maybe mention it in the commit message. With this: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
On 10/7/20 5:53 AM, Kevin Wolf wrote: > Am 07.10.2020 um 01:58 hat John Snow geschrieben: >> Like many other Optional[] types, it's not always a given that this >> object will be set. Wrap it in a type-shim that raises a meaningful >> error and will always return a concrete type. >> >> Signed-off-by: John Snow <jsnow@redhat.com> > >> @@ -515,11 +515,13 @@ def set_qmp_monitor(self, enabled=True): >> line. Default is True. >> @note: call this function before launch(). >> """ >> - if enabled: >> - self._qmp_set = True >> - else: >> - self._qmp_set = False >> - self._qmp = None >> + self._qmp_set = enabled > > This change seems unrelated to wrapping the connection in a property. > Intuitively, it makes sense that the connection of a running instance > doesn't go away just because I disable QMP in the command line for the > next launch. > > If this is the reasoning behind the change, maybe mention it in the > commit message. > > With this: > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > Oh, yes. That's what happened here -- and it got folded in here specifically to make that access check consistent. I'll update the commit message.
diff --git a/python/qemu/machine.py b/python/qemu/machine.py index d788e8aba8c..3e9cf09fd2d 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -135,7 +135,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None, self._events = [] self._iolog = None self._qmp_set = True # Enable QMP monitor by default. - self._qmp = None + self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None self._qemu_full_args = None self._temp_dir = None self._launched = False @@ -302,14 +302,14 @@ def _pre_launch(self): if self._remove_monitor_sockfile: assert isinstance(self._monitor_address, str) self._remove_files.append(self._monitor_address) - self._qmp = qmp.QEMUMonitorProtocol( + self._qmp_connection = qmp.QEMUMonitorProtocol( self._monitor_address, server=True, nickname=self._name ) def _post_launch(self): - if self._qmp: + if self._qmp_connection: self._qmp.accept() def _post_shutdown(self): @@ -320,9 +320,9 @@ def _post_shutdown(self): # Comprehensive reset for the failed launch case: self._early_cleanup() - if self._qmp: + if self._qmp_connection: self._qmp.close() - self._qmp = None + self._qmp_connection = None self._load_io_log() @@ -434,7 +434,7 @@ def _soft_shutdown(self, timeout: Optional[int], """ self._early_cleanup() - if self._qmp is not None: + if self._qmp_connection: if not has_quit: # Might raise ConnectionReset self._qmp.cmd('quit') @@ -515,11 +515,13 @@ def set_qmp_monitor(self, enabled=True): line. Default is True. @note: call this function before launch(). """ - if enabled: - self._qmp_set = True - else: - self._qmp_set = False - self._qmp = None + self._qmp_set = enabled + + @property + def _qmp(self) -> qmp.QEMUMonitorProtocol: + if self._qmp_connection is None: + raise QEMUMachineError("Attempt to access QMP with no connection") + return self._qmp_connection @classmethod def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
Like many other Optional[] types, it's not always a given that this object will be set. Wrap it in a type-shim that raises a meaningful error and will always return a concrete type. Signed-off-by: John Snow <jsnow@redhat.com> --- python/qemu/machine.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)