Re: [PATCH 1/2] python/machine.py: upgrade vm.command() method

On Thu, May 26, 2022, 10:31 AM Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:

On 4/8/22 20:02, Vladimir Sementsov-Ogievskiy wrote:
> The method is not popular, we prefer use vm.qmp() and then check

Suddenly I found, that I missed a lot of existing users: in scripts, in avocado tests.

Do you prefer to rename the method to "cmd()", and change all the occurrences, or keep longer "command()" name and update the second patch?

I don't have a strong preference, I think.

In (async) qmp I use .execute() as the form that raises exception, and ._raw() as the form that doesn't. I use execute_msg() as an exception-raising form that takes a Mapping[str, obj].

Notably, I tried to hide any interface that didn't raise exception, and the interfaces that remain always return the inner return field and not the entire wire object.

command() IIRC has historically been the exception-raising version and cmd() has been the C-like version. (cmd_obj works like my execute_msg, except it doesn't raise, and returns the entire reply.)

command() is longer, but there's precedent and continuity for it working this way. But shorter names are nicer for line length, so...

...Go with what you feel is subjectively nicest? (That's not helpful, sorry.)

oh, also: IIRC, command() also does not return the entire response object. This is how execute() works, but it might be a lot of churn to convert users of cmd() over to this form. It's something I want to do eventually anyway, but it's a lot for me to dump on your plate, so don't worry about that aspect.

--js

$ git grep '\.command('
docs/devel/testing.rst: res = self.vm.command('human-monitor-command',
docs/devel/testing.rst: first_res = first_machine.command(
docs/devel/testing.rst: second_res = second_machine.command(
docs/devel/testing.rst: third_res = self.get_vm(name='third_machine').command(
python/qemu/machine/machine.py: ret = self._qmp.command(cmd, **qmp_args)
python/qemu/utils/qemu_ga_client.py: return self.command('guest-' + name.replace('_', '-'), **kwds)
python/qemu/utils/qom.py: rsp = self.qmp.command(
python/qemu/utils/qom.py: rsp = self.qmp.command(
python/qemu/utils/qom.py: rsp = self.qmp.command('qom-get', path=path,
python/qemu/utils/qom_common.py: rsp = self.qmp.command('qom-list', path=path)
python/qemu/utils/qom_fuse.py: data = "" path=path, property=prop))
python/qemu/utils/qom_fuse.py: return prefix + str(self.qmp.command('qom-get', path=path,
scripts/device-crash-test: types = vm.command('qom-list-types', **kwargs)
scripts/device-crash-test: devhelp = vm.command('human-monitor-command', **args)
scripts/device-crash-test: self.machines = list(m['name'] for m in vm.command('query-machines'))
scripts/device-crash-test: self.kvm_available = vm.command('query-kvm')['enabled']
scripts/render_block_graph.py: bds_nodes = qmp.command('query-named-block-nodes')
scripts/render_block_graph.py: job_nodes = qmp.command('query-block-jobs')
scripts/render_block_graph.py: block_graph = qmp.command('x-debug-query-block-graph')
tests/avocado/avocado_qemu/__init__.py: res = self.vm.command('human-monitor-command',
tests/avocado/cpu_queries.py: cpus = self.vm.command('query-cpu-definitions')
tests/avocado/cpu_queries.py: e = self.vm.command('query-cpu-model-expansion', model=model, type='full')
tests/avocado/hotplug_cpu.py: self.vm.command('device_add',
tests/avocado/info_usernet.py: res = self.vm.command('human-monitor-command',
tests/avocado/machine_arm_integratorcp.py: self.vm.command('human-monitor-command', command_line='stop')
tests/avocado/machine_arm_integratorcp.py: self.vm.command('human-monitor-command',
tests/avocado/machine_m68k_nextcube.py: self.vm.command('human-monitor-command',
tests/avocado/machine_mips_malta.py: self.vm.command('human-monitor-command', command_line='stop')
tests/avocado/machine_mips_malta.py: self.vm.command('human-monitor-command',
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('device_del', id='rn1')
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('device_del', id='rn2')
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('device_add', driver='virtio-net-ccw',
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('device_del', id='net_4711')
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('human-monitor-command', command_line='balloon 96')
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('human-monitor-command', command_line='balloon 128')
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('screendump', filename=ppmfile.name)
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('object-add', qom_type='cryptodev-backend-builtin',
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('device_add', driver='virtio-crypto-ccw', id='crypdev0',
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('device_del', id='crypdev0')
tests/avocado/machine_s390_ccw_virtio.py: self.vm.command('object-del', id='cbe0')
tests/avocado/migration.py: return vm.command('query-migrate')['status'] in ('completed', 'failed')
tests/avocado/migration.py: self.assertEqual(src_vm.command('query-migrate')['status'], 'completed')
tests/avocado/migration.py: self.assertEqual(dst_vm.command('query-migrate')['status'], 'completed')
tests/avocado/migration.py: self.assertEqual(dst_vm.command('query-status')['status'], 'running')
tests/avocado/migration.py: self.assertEqual(src_vm.command('query-status')['status'],'postmigrate')
tests/avocado/pc_cpu_hotplug_props.py: self.assertEquals(len(self.vm.command('query-cpus-fast')), 2)
tests/avocado/version.py: res = self.vm.command('human-monitor-command',
tests/avocado/virtio_check_params.py: output = vm.command('human-monitor-command',
tests/avocado/virtio_check_params.py: machines = [m['name'] for m in vm.command('query-machines')]
tests/avocado/virtio_version.py: return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)]
tests/avocado/virtio_version.py: pcibuses = vm.command('query-pci')
tests/avocado/x86_cpu_model_versions.py: cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
tests/avocado/x86_cpu_model_versions.py: cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
tests/avocado/x86_cpu_model_versions.py: cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
tests/avocado/x86_cpu_model_versions.py: cpu_path = self.vm.command('query-cpus-fast')[0].get('qom-path')
tests/avocado/x86_cpu_model_versions.py: return self.vm.command('qom-get', path=cpu_path, property=prop)
tests/docker/docker.py: dkr.command(cmd="pull", quiet=args.quiet,
tests/docker/docker.py: dkr.command(cmd="tag", quiet=args.quiet,
tests/docker/docker.py: return Docker().command("images", argv, args.quiet)
tests/migration/guestperf/engine.py: info = vm.command("query-migrate")
tests/migration/guestperf/engine.py: vcpus = src.command("query-cpus-fast")
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = dst.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = dst.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = dst.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = dst.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = src.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = dst.command("migrate-set-capabilities",
tests/migration/guestperf/engine.py: resp = dst.command("migrate-set-parameters",
tests/migration/guestperf/engine.py: resp = src.command("migrate", uri=connect_uri)
tests/migration/guestperf/engine.py: dst.command("cont")
tests/migration/guestperf/engine.py: src.command("migrate_cancel")
tests/migration/guestperf/engine.py: src.command("migrate_cancel")
tests/migration/guestperf/engine.py: resp = src.command("migrate-start-postcopy")
tests/migration/guestperf/engine.py: resp = src.command("stop")

> success by hand.. But that's not optimal. To simplify movement to
> vm.command() support same interface improvements like in vm.qmp() and
> rename to shorter vm.cmd().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
> python/qemu/machine/machine.py | 16 ++++++++++++---
> tests/qemu-iotests/256 | 34 ++++++++++++++++----------------
> tests/qemu-iotests/257 | 36 +++++++++++++++++-----------------
> 3 files changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 07ac5a710b..a3fb840b93 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -648,14 +648,24 @@ def qmp(self, cmd: str,
> self._quit_issued = True
> return ret
>
> - def command(self, cmd: str,
> - conv_keys: bool = True,
> - **args: Any) -> QMPReturnValue:
> + def cmd(self, cmd: str,
> + args_dict: Optional[Dict[str, object]] = None,
> + conv_keys: Optional[bool] = None,
> + **args: Any) -> QMPReturnValue:
> """
> Invoke a QMP command.
> On success return the response dict.
> On failure raise an exception.
> """
> + if args_dict is not None:
> + assert not args
> + assert conv_keys is None
> + args = args_dict
> + conv_keys = False
> +
> + if conv_keys is None:
> + conv_keys = True
> +
> qmp_args = self._qmp_args(conv_keys, args)
> ret = self._qmp.command(cmd, **qmp_args)
> if cmd == 'quit':
> diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
> index 13666813bd..fffc8ef055 100755
> --- a/tests/qemu-iotests/256
> +++ b/tests/qemu-iotests/256
> @@ -40,25 +40,25 @@ with iotests.FilePath('img0') as img0_path, \
> def create_target(filepath, name, size):
> basename = os.path.basename(filepath)
> nodename = "file_{}".format(basename)
> - log(vm.command('blockdev-create', job_id='job1',
> - options={
> - 'driver': 'file',
> - 'filename': filepath,
> - 'size': 0,
> - }))
> + log(vm.cmd('blockdev-create', job_id='job1',
> + options={
> + 'driver': 'file',
> + 'filename': filepath,
> + 'size': 0,
> + }))
> vm.run_job('job1')
> - log(vm.command('blockdev-add', driver='file',
> - node_name=nodename, filename=filepath))
> - log(vm.command('blockdev-create', job_id='job2',
> - options={
> - 'driver': iotests.imgfmt,
> - 'file': nodename,
> - 'size': size,
> - }))
> + log(vm.cmd('blockdev-add', driver='file',
> + node_name=nodename, filename=filepath))
> + log(vm.cmd('blockdev-create', job_id='job2',
> + options={
> + 'driver': iotests.imgfmt,
> + 'file': nodename,
> + 'size': size,
> + }))
> vm.run_job('job2')
> - log(vm.command('blockdev-add', driver=iotests.imgfmt,
> - node_name=name,
> - file=nodename))
> + log(vm.cmd('blockdev-add', driver=iotests.imgfmt,
> + node_name=name,
> + file=nodename))
>
> log('--- Preparing images & VM ---\n')
> vm.add_object('iothread,id=iothread0')
> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
> index e7e7a2317e..7d3720b8e5 100755
> --- a/tests/qemu-iotests/257
> +++ b/tests/qemu-iotests/257
> @@ -160,26 +160,26 @@ class Drive:
> file_node_name = "file_{}".format(basename)
> vm = self.vm
>
> - log(vm.command('blockdev-create', job_id='bdc-file-job',
> - options={
> - 'driver': 'file',
> - 'filename': self.path,
> - 'size': 0,
> - }))
> + log(vm.cmd('blockdev-create', job_id='bdc-file-job',
> + options={
> + 'driver': 'file',
> + 'filename': self.path,
> + 'size': 0,
> + }))
> vm.run_job('bdc-file-job')
> - log(vm.command('blockdev-add', driver='file',
> - node_name=file_node_name, filename=self.path))
> -
> - log(vm.command('blockdev-create', job_id='bdc-fmt-job',
> - options={
> - 'driver': fmt,
> - 'file': file_node_name,
> - 'size': size,
> - }))
> + log(vm.cmd('blockdev-add', driver='file',
> + node_name=file_node_name, filename=self.path))
> +
> + log(vm.cmd('blockdev-create', job_id='bdc-fmt-job',
> + options={
> + 'driver': fmt,
> + 'file': file_node_name,
> + 'size': size,
> + }))
> vm.run_job('bdc-fmt-job')
> - log(vm.command('blockdev-add', driver=fmt,
> - node_name=name,
> - file=file_node_name))
> + log(vm.cmd('blockdev-add', driver=fmt,
> + node_name=name,
> + file=file_node_name))
> self.fmt = fmt
> self.size = size
> self.node = name

--
Best regards,
Vladimir

From:	John Snow
Subject:	Re: [PATCH 1/2] python/machine.py: upgrade vm.command() method
Date:	Thu, 26 May 2022 10:55:54 -0400