Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim

[Kevin Wolf <kwolf@redhat.com>]

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