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

[John Snow <jsnow@redhat.com>]

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é
>>> <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?
>>

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