| Message ID | 20200604202236.25039-13-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python: add mypy support to python/qemu | expand |
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)
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?
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? Kevin
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.
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é >> <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? > > 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
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
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]:
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(-)