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

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 15787 bytes --]

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 =
> str(self.qmp.command('qom-get', 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
>

[-- Attachment #2: Type: text/html, Size: 21122 bytes --]