[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: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim |
Date: |
Mon, 22 Jun 2020 13:32:40 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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.
- [PATCH v3 05/16] python/qmp.py: add casts to JSON deserialization, (continued)
- [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
Re: [PATCH v3 00/16] python: add mypy support to python/qemu, Kevin Wolf, 2020/06/05