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

[Philippe Mathieu-Daudé]

On 6/22/20 12:23 PM, Kevin Wolf wrote:
> 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?

Better indeed.

John, at this point from a maintenance perspective it is easier
if you respin the series (and please, run the Travis-CI tests).

Regards,

Phil.