Re: [PATCH v4 01/11] python: rename QEMUMonitorProtocol.cmd() to cmd_raw()

[Daniel P . Berrangé]

On Tue, Jan 10, 2023 at 11:37:48AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Having cmd() and command() methods in one class doesn't look good.
> Rename cmd() to cmd_raw(), to show its meaning better.
> 
> We also want to rename command() to cmd() in future, so this commit is a
> necessary first step.
> 
> Keep new cmd_raw() only in a few places where it's really needed and
> move to command() where possible.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  python/qemu/machine/machine.py |  2 +-
>  python/qemu/qmp/legacy.py      |  8 ++------
>  python/qemu/qmp/qmp_shell.py   | 13 +++++++------
>  scripts/cpu-x86-uarch-abi.py   | 12 ++++++------
>  tests/qemu-iotests/iotests.py  |  2 +-
>  5 files changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 748a0d807c..9059dc3948 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -674,7 +674,7 @@ def qmp(self, cmd: str,
>              conv_keys = True
>  
>          qmp_args = self._qmp_args(conv_keys, args)
> -        ret = self._qmp.cmd(cmd, args=qmp_args)
> +        ret = self._qmp.cmd_raw(cmd, args=qmp_args)
>          if cmd == 'quit' and 'error' not in ret and 'return' in ret:
>              self._quit_issued = True
>          return ret
> diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py
> index 1951754455..8e1a504052 100644
> --- a/python/qemu/qmp/legacy.py
> +++ b/python/qemu/qmp/legacy.py
> @@ -186,21 +186,17 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
>              )
>          )
>  
> -    def cmd(self, name: str,
> -            args: Optional[Dict[str, object]] = None,
> -            cmd_id: Optional[object] = None) -> QMPMessage:
> +    def cmd_raw(self, name: str,
> +            args: Optional[Dict[str, object]] = None) -> QMPMessage:
>          """
>          Build a QMP command and send it to the QMP Monitor.
>  
>          :param name: command name (string)
>          :param args: command arguments (dict)
> -        :param cmd_id: command id (dict, list, string or int)
>          """
>          qmp_cmd: QMPMessage = {'execute': name}
>          if args:
>              qmp_cmd['arguments'] = args
> -        if cmd_id:
> -            qmp_cmd['id'] = cmd_id

The commit message didn't say anything about dropping the
cmd_id parameter. Presumably you've found that it is not
used in any caller that exists, but still it feels like
a valid parameter to want to support in this method ?


>          return self.cmd_obj(qmp_cmd)
>  
>      def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
> diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
> index 619ab42ced..5c0d87a0ec 100644
> --- a/python/qemu/qmp/qmp_shell.py
> +++ b/python/qemu/qmp/qmp_shell.py
> @@ -98,7 +98,7 @@
>      Sequence,
>  )
>  
> -from qemu.qmp import ConnectError, QMPError, SocketAddrT
> +from qemu.qmp import ConnectError, QMPError, SocketAddrT, ExecuteError
>  from qemu.qmp.legacy import (
>      QEMUMonitorProtocol,
>      QMPBadPortError,
> @@ -194,11 +194,12 @@ def close(self) -> None:
>          super().close()
>  
>      def _fill_completion(self) -> None:
> -        cmds = self.cmd('query-commands')
> -        if 'error' in cmds:
> -            return
> -        for cmd in cmds['return']:
> -            self._completer.append(cmd['name'])
> +        try:
> +            cmds = self.command('query-commands')
> +            for cmd in cmds:
> +                self._completer.append(cmd['name'])
> +        except ExecuteError:
> +            pass
>  
>      def _completer_setup(self) -> None:
>          self._completer = QMPCompleter()

I'd suggest that re-writing callers to use 'command' is better
done in a prior patch, so that this patch is purely a rename of
cmd -> cmd_raw with no functional changes intermixed.

> diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
> index 82ff07582f..893afd1b35 100644
> --- a/scripts/cpu-x86-uarch-abi.py
> +++ b/scripts/cpu-x86-uarch-abi.py
> @@ -69,7 +69,7 @@
>  shell = QEMUMonitorProtocol(sock)
>  shell.connect()
>  
> -models = shell.cmd("query-cpu-definitions")
> +models = shell.command("query-cpu-definitions")
>  
>  # These QMP props don't correspond to CPUID fatures
>  # so ignore them
> @@ -85,7 +85,7 @@
>  
>  names = []
>  
> -for model in models["return"]:
> +for model in models:
>      if "alias-of" in model:
>          continue
>      names.append(model["name"])
> @@ -93,12 +93,12 @@
>  models = {}
>  
>  for name in sorted(names):
> -    cpu = shell.cmd("query-cpu-model-expansion",
> -                     { "type": "static",
> -                       "model": { "name": name }})
> +    cpu = shell.command("query-cpu-model-expansion",
> +                        { "type": "static",
> +                          "model": { "name": name }})
>  
>      got = {}
> -    for (feature, present) in cpu["return"]["model"]["props"].items():
> +    for (feature, present) in cpu["model"]["props"].items():
>          if present and feature not in skip:
>              got[feature] = True
>  
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index da7d6637e1..c69b10ac82 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -460,7 +460,7 @@ def __init__(self, *args: str, instance_id: str = 'a', 
> qmp: bool = False):
>      def qmp(self, cmd: str, args: Optional[Dict[str, object]] = None) \
>              -> QMPMessage:
>          assert self._qmp is not None
> -        return self._qmp.cmd(cmd, args)
> +        return self._qmp.cmd_raw(cmd, args)
>  
>      def stop(self, kill_signal=15):
>          self._p.send_signal(kill_signal)
> -- 
> 2.34.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|