On 6/22/20 6:23 AM, 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é
>> <phi...@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 <js...@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
>
Using Optional[qmp.QEMUMonitorProtocol] does work; it does require a lot
more "if not None" checks littered everywhere.
>From memory, it was just difficult to type a qmp(...) passthrough
command that integrated the "if self._qmp_socket" check -- the faster
hack was the shim around accessing the socket itself.
IIRC, mypy had difficulty reasoning about constraints otherwise -- but
I'll look into it again. At this point it warrants stronger effort.
--js
