On 6/22/20 7:32 AM, Philippe Mathieu-Daudé wrote:
> 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é
>>> <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?
>>
Not entirely; the idea is to protect access to the socket in all cases
where the socket was not created, for any and all reasons.
>> 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.
>>
The _qmp_connection variable is already Optional[], so you could indeed
remove the shim. The shim is here to peel away the Optional[] so you
don't need the "if qmp" checks everywhere.
>> 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?
>>
Yup. The code in lifetime management functions should check for the
socket. Code in qmp-centric commands should assume it's there with the shim.
>> Or maybe going a step back, why do we even have a separate _qmp_set
>> instead of only using None for _qmp?
Because the socket isn't initialized until after launch, so "_qmp_set"
is a request/instruction and not reflective of current state.
_qmp_set is the flag for a desire to create QMP or not,
_qmp_connection is the actual connection.
set | conn |
T F | OK (we are pre-launch)
T T | OK (we are running)
F F | OK (we set no QMP before launch)
F T | Set QMP false after launch
The last one doesn't make a lot of sense. Worse, it just None's the
socket variable so we can't even close it properly.
That's sorta busted, so I am patching it to just simply set the variable.
>
> 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.
>
OK, I am resending. I didn't make any big changes, but I thought about it.
(Read as: I tried, didn't like how it looked, and came back to this
patchset which is fairly close as-is.)
--js
