machine.py and qmp.py both do the same thing here; refactor machine.py
to use qmp.py's functionality more directly.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index 61ee3a0e81..34e6b6f9e9 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -25,6 +25,8 @@
 import socket
 import tempfile
 from typing import (
+    Any,
+    Dict,
     List,
     Optional,
     Type,
@@ -416,17 +418,23 @@ def set_qmp_monitor(self, enabled=True):
             self._qmp_set = False
             self._qmp = None
 
-    def qmp(self, cmd, conv_keys=True, **args):
-        """
-        Invoke a QMP command and return the response dict
-        """
+    @classmethod
+    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
         qmp_args = dict()
         for key, value in args.items():
-            if conv_keys:
+            if _conv_keys:
                 qmp_args[key.replace('_', '-')] = value
             else:
                 qmp_args[key] = value
+        return qmp_args
 
+    def qmp(self, cmd: str,
+            conv_keys: bool = True,
+            **args: Any) -> QMPMessage:
+        """
+        Invoke a QMP command and return the response dict
+        """
+        qmp_args = self._qmp_args(conv_keys, **args)
         return self._qmp.cmd(cmd, args=qmp_args)
 
     def command(self, cmd, conv_keys=True, **args):
@@ -435,12 +443,8 @@ def command(self, cmd, conv_keys=True, **args):
         On success return the response dict.
         On failure raise an exception.
         """
-        reply = self.qmp(cmd, conv_keys, **args)
-        if reply is None:
-            raise qmp.QMPError("Monitor is closed")
-        if "error" in reply:
-            raise qmp.QMPResponseError(reply)
-        return reply["return"]
+        qmp_args = self._qmp_args(conv_keys, **args)
+        return self._qmp.command(cmd, **qmp_args)
 
     def get_qmp_event(self, wait=False):
         """
-- 
2.21.1

[...]

>  
> -    def qmp(self, cmd, conv_keys=True, **args):
> -        """
> -        Invoke a QMP command and return the response dict
> -        """
> +    @classmethod
> +    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
>          qmp_args = dict()
>          for key, value in args.items():
> -            if conv_keys:
> +            if _conv_keys:
>                  qmp_args[key.replace('_', '-')] = value
>              else:
>                  qmp_args[key] = value
> +        return qmp_args
>  
> +    def qmp(self, cmd: str,
> +            conv_keys: bool = True,
> +            **args: Any) -> QMPMessage:

This creates an interesting problem with iotests 297:


-Success: no issues found in 1 source file
+iotests.py:563: error: Argument 2 to "qmp" of "QEMUMachine" has
incompatible type "**Dict[str, str]"; expected "bool"
+Found 1 error in 1 file (checked 1 source file)


def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
    cmd = 'human-monitor-command'
    kwargs = {'command-line': command_line}
    if use_log:
        return self.qmp_log(cmd, **kwargs)
    else:
        return self.qmp(cmd, **kwargs)

It seems like mypy is unable to understand that we are passing keyword
arguments, and instead believes we're passing something to the conv_keys
parameter.

(Is this a bug...?)

Even amending the function signature to indicate that conv_keys should
only ever appear as a keyword argument doesn't seem to help.

I'll have to think about a nice way to fix this; removing conv_keys out
of the argument namespace seems like the best approach.

qmp(cmd, foo=bar, hello=world)
qmp(cmd, **conv_keys(foo=bar, hello=world))

...but now this function looks really annoying to call.

Uh, I'll play around with this, but let me know if you have any cool ideas.

--js

Am 29.05.2020 um 02:18 hat John Snow geschrieben:
> 
> [...]
> 
> >  
> > -    def qmp(self, cmd, conv_keys=True, **args):
> > -        """
> > -        Invoke a QMP command and return the response dict
> > -        """
> > +    @classmethod
> > +    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
> >          qmp_args = dict()
> >          for key, value in args.items():
> > -            if conv_keys:
> > +            if _conv_keys:
> >                  qmp_args[key.replace('_', '-')] = value
> >              else:
> >                  qmp_args[key] = value
> > +        return qmp_args
> >  
> > +    def qmp(self, cmd: str,
> > +            conv_keys: bool = True,
> > +            **args: Any) -> QMPMessage:
> 
> This creates an interesting problem with iotests 297:
> 
> 
> -Success: no issues found in 1 source file
> +iotests.py:563: error: Argument 2 to "qmp" of "QEMUMachine" has
> incompatible type "**Dict[str, str]"; expected "bool"
> +Found 1 error in 1 file (checked 1 source file)
> 
> 
> def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>     cmd = 'human-monitor-command'
>     kwargs = {'command-line': command_line}
>     if use_log:
>         return self.qmp_log(cmd, **kwargs)
>     else:
>         return self.qmp(cmd, **kwargs)
> 
> It seems like mypy is unable to understand that we are passing keyword
> arguments, and instead believes we're passing something to the conv_keys
> parameter.
> 
> (Is this a bug...?)

It took some experimenting, but in the end, I think the logic behind it
is reasonable enough. If you have kwargs: Dict[str, T] and pass it as
**kwargs, mypy checks that passing T to _every_ keyword argument is fine
because generally speaking you have to expect that the name of any
argument could be present in kwargs. (In the specific example, of
course, we know all the keys, but mypy doesn't look at the content of
the dict.)

With this in mind, getting rid of the error is very simple because you
only need to make sure that T is Any instead of str:

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7eb714b8e5..0258f1359a 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -556,7 +556,7 @@ class VM(qtest.QEMUQtestMachine):
 
     def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
         cmd = 'human-monitor-command'
-        kwargs = {'command-line': command_line}
+        kwargs: Dict[str, Any] = {'command-line': command_line}
         if use_log:
             return self.qmp_log(cmd, **kwargs)
         else:

Kevin

Am 02.06.2020 um 12:18 hat Kevin Wolf geschrieben:
> Am 29.05.2020 um 02:18 hat John Snow geschrieben:
> > 
> > [...]
> > 
> > >  
> > > -    def qmp(self, cmd, conv_keys=True, **args):
> > > -        """
> > > -        Invoke a QMP command and return the response dict
> > > -        """
> > > +    @classmethod
> > > +    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
> > >          qmp_args = dict()
> > >          for key, value in args.items():
> > > -            if conv_keys:
> > > +            if _conv_keys:
> > >                  qmp_args[key.replace('_', '-')] = value
> > >              else:
> > >                  qmp_args[key] = value
> > > +        return qmp_args
> > >  
> > > +    def qmp(self, cmd: str,
> > > +            conv_keys: bool = True,
> > > +            **args: Any) -> QMPMessage:
> > 
> > This creates an interesting problem with iotests 297:
> > 
> > 
> > -Success: no issues found in 1 source file
> > +iotests.py:563: error: Argument 2 to "qmp" of "QEMUMachine" has
> > incompatible type "**Dict[str, str]"; expected "bool"
> > +Found 1 error in 1 file (checked 1 source file)
> > 
> > 
> > def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
> >     cmd = 'human-monitor-command'
> >     kwargs = {'command-line': command_line}
> >     if use_log:
> >         return self.qmp_log(cmd, **kwargs)
> >     else:
> >         return self.qmp(cmd, **kwargs)
> > 
> > It seems like mypy is unable to understand that we are passing keyword
> > arguments, and instead believes we're passing something to the conv_keys
> > parameter.
> > 
> > (Is this a bug...?)
> 
> It took some experimenting, but in the end, I think the logic behind it
> is reasonable enough. If you have kwargs: Dict[str, T] and pass it as
> **kwargs, mypy checks that passing T to _every_ keyword argument is fine
> because generally speaking you have to expect that the name of any
> argument could be present in kwargs. (In the specific example, of
> course, we know all the keys, but mypy doesn't look at the content of
> the dict.)
> 
> With this in mind, getting rid of the error is very simple because you
> only need to make sure that T is Any instead of str:
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7eb714b8e5..0258f1359a 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -556,7 +556,7 @@ class VM(qtest.QEMUQtestMachine):
>  
>      def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>          cmd = 'human-monitor-command'
> -        kwargs = {'command-line': command_line}
> +        kwargs: Dict[str, Any] = {'command-line': command_line}
>          if use_log:
>              return self.qmp_log(cmd, **kwargs)
>          else:

There is actually also a second way to get rid of the error, which is
specifying conv_keys explicitly so that the value from kwargs wouldn't
be used (if anything, we would get an exception because the value was
specified twice). So the patch below makes the mypy error go away, too.

I think I prefer the explicit Dict[str, Any] because it doesn't end up
duplicating the default value, but I just wanted to mention that this
option exists, too.

Kevin


diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7eb714b8e5..86b14f6152 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -560,7 +560,7 @@ class VM(qtest.QEMUQtestMachine):
         if use_log:
             return self.qmp_log(cmd, **kwargs)
         else:
-            return self.qmp(cmd, **kwargs)
+            return self.qmp(cmd, True, **kwargs)
 
     def pause_drive(self, drive: str, event: Optional[str] = None) -> None:
         """Pause drive r/w operations"""

On 6/2/20 6:26 AM, Kevin Wolf wrote:
> I think I prefer the explicit Dict[str, Any] because it doesn't end up
> duplicating the default value, but I just wanted to mention that this
> option exists, too.

Oh, I see what's going on here now! Thanks, that's very helpful! In this
case, for now, I think I like your first solution, too.

Someday in the future, I want to look into strictly typed generated SDK
objects where mypy *does* know the types of every field in a QMP command
object.

That's not today, and it's probably not soon, (and it might be never,)
but until then, explicitly hinting the kwargs to be Dict[str, Any] is
reasonable enough.

Thank you so much!

--js