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 <vsement...@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 :|
