[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim |
Date: |
Mon, 22 Jun 2020 12:23:42 +0200 |
Am 20.06.2020 um 10:20 hat Philippe Mathieu-Daudé geschrieben:
> On Sat, Jun 20, 2020 at 10:14 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > On 6/4/20 10:22 PM, John Snow wrote:
> > > 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 | 12 +++++++++---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > > index d8289936816..a451f9000d6 100644
> > > --- a/python/qemu/machine.py
> > > +++ b/python/qemu/machine.py
> > > @@ -118,7 +118,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
> > > @@ -285,7 +285,7 @@ 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
> > > @@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
> > > self._qmp_set = True
> > > else:
> > > self._qmp_set = False
> > > - self._qmp = None
> > > + self._qmp_connection = None
> > > +
> > > + @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]:
> > >
> >
> > This patch breaks the EmptyCPUModel test:
> >
> > (043/101) tests/acceptance/empty_cpu_model.py:EmptyCPUModel.test:
> > ERROR: Attempt to access QMP with no connection (0.03 s)
>
> Fixed with:
>
> -- >8 --
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index ba6397dd7e..26ae7be89b 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -480,7 +480,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
>
> @property
> def _qmp(self) -> qmp.QEMUMonitorProtocol:
> - if self._qmp_connection is None:
> + if self._qmp_set and self._qmp_connection is None:
> raise QEMUMachineError("Attempt to access QMP with no
> connection")
> return self._qmp_connection
>
> ---
>
> Does that sound reasonable to you?
Wouldn't that make the return type Optional[qmp.QEMUMonitorProtocol]?
Maybe this is what we want, but then we don't need the shim that this
patch adds but can just declare the variable this way.
And why does the feeling code even try to acess _qmp when _qmp_set is
False? Shouldn't it first check whether it's even valid?
Or maybe going a step back, why do we even have a separate _qmp_set
instead of only using None for _qmp?
Kevin
- [PATCH v3 03/16] python/qmp.py: re-absorb MonitorResponseError, (continued)
- [PATCH v3 03/16] python/qmp.py: re-absorb MonitorResponseError, John Snow, 2020/06/04
- [PATCH v3 05/16] python/qmp.py: add casts to JSON deserialization, John Snow, 2020/06/04
- [PATCH v3 06/16] python/qmp.py: add QMPProtocolError, John Snow, 2020/06/04
- [PATCH v3 02/16] iotests.py: use qemu.qmp type aliases, John Snow, 2020/06/04
- [PATCH v3 08/16] python/machine.py: reorder __init__, John Snow, 2020/06/04
- [PATCH v3 07/16] python/machine.py: Fix monitor address typing, John Snow, 2020/06/04
- [PATCH v3 09/16] python/machine.py: Don't modify state in _base_args(), John Snow, 2020/06/04
- [PATCH v3 12/16] python/machine.py: Add _qmp access shim, John Snow, 2020/06/04
[PATCH v3 10/16] python/machine.py: Handle None events in events_wait, John Snow, 2020/06/04
[PATCH v3 11/16] python/machine.py: use qmp.command, John Snow, 2020/06/04
[PATCH v3 15/16] iotests.py: Adjust HMP kwargs typing, John Snow, 2020/06/04
[PATCH v3 13/16] python/machine.py: fix _popen access, John Snow, 2020/06/04
[PATCH v3 14/16] python/qemu: make 'args' style arguments immutable, John Snow, 2020/06/04
[PATCH v3 16/16] python/qemu: Add mypy type annotations, John Snow, 2020/06/04
[PATCH v3 01/16] python/qmp.py: Define common types, John Snow, 2020/06/04