Re: [PATCH v2 03/29] python/qemu: Add binutils::binary_get_version()

From: Wainer dos Santos Moschetta <wainersm@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Denis Plotnikov" <dplotnikov@virtuozzo.com>,
	qemu-devel@nongnu.org
Cc: Cornelia Huck <cohuck@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH v2 03/29] python/qemu: Add binutils::binary_get_version()
Date: Mon, 3 Feb 2020 17:17:49 -0200	[thread overview]
Message-ID: <cfec25d9-95c4-18e9-f4e1-d087a7bc5a2e@redhat.com> (raw)
In-Reply-To: <20200129212345.20547-4-philmd@redhat.com>

On 1/29/20 7:23 PM, Philippe Mathieu-Daudé wrote:
> Add a helper to query the version of a QEMU binary.
> We simply send the 'query-version' command over a QMP
> socket.
> Introduce the PythonQemuCoreScripts class to test our
> new helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   python/qemu/binutils.py          | 38 ++++++++++++++++++++++++++++++++

I'm not sure about creating the file with that name, it reminds me to 
GNU Binutils rather than the QEMU binary. :)

Perhaps it could be named qemu_bin.py?

Another suggestion is to encapsulate the methods you propose in this 
series in an object. For example:

class QEMUBin:
     def __init__(self, bin_path):
         # Check bin exists.
         self.bin_path = bin_path

     def get_version(self):
         # binutils.binary_get_version() goes here.
         pass

     def get_arch(self):
         # binutils.binary_get_arch() goes here.
         pass

     def list_accel(self):
         # move accel.list_accel() method to here.
         pass

     def get_vm(self, args):
         # Return an QEMUMachine object...
         return QEMUMachine(self.bin_path, *args)

     def get_build_config_host(self):
         # Detect if self.bin_path is in a build directory,
         # attempt to read the host-config.mak and return
         # as hash. Or fail...
         pass

>   tests/acceptance/core_scripts.py | 31 ++++++++++++++++++++++++++
>   2 files changed, 69 insertions(+)
>   create mode 100644 python/qemu/binutils.py
>   create mode 100644 tests/acceptance/core_scripts.py
>
> diff --git a/python/qemu/binutils.py b/python/qemu/binutils.py
> new file mode 100644
> index 0000000000..96b200eef4
> --- /dev/null
> +++ b/python/qemu/binutils.py
> @@ -0,0 +1,38 @@
> +"""
> +QEMU binary utility module:
> +
> +The binary utility module provides helpers to query QEMU binary for
> +build-dependent configuration options at runtime.
> +"""
> +#
> +# Copyright (c) 2020 Red Hat, Inc.
> +#
> +# Author:
> +#  Philippe Mathieu-Daudé <philmd@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import logging
> +
> +from .machine import QEMUMachine
> +
> +LOG = logging.getLogger(__name__)
> +
> +
> +def binary_get_version(qemu_bin):
> +    '''
> +    Get QEMU binary version
> +
> +    @param qemu_bin (str): path to the QEMU binary

It could check that qemu_bin file exists, otherwise raise an exception 
or return None.

> +    @return binary version (dictionary with major/minor/micro keys)
> +    '''
> +    with QEMUMachine(qemu_bin) as vm:
> +        vm.set_machine('none')
> +        vm.launch()
> +        res = vm.command('query-version')
> +        LOG.info(res)
> +        vm.shutdown()

Don't need this, the vm will be shutdown anyway (see QEMUMachine.__exit__())

Thanks!

- Wainer

> +        return res['qemu']
> diff --git a/tests/acceptance/core_scripts.py b/tests/acceptance/core_scripts.py
> new file mode 100644
> index 0000000000..3f253337cd
> --- /dev/null
> +++ b/tests/acceptance/core_scripts.py
> @@ -0,0 +1,31 @@
> +# Tests covering various python/qemu/ scripts
> +#
> +# Copyright (c) 2020 Red Hat, Inc.
> +#
> +# Author:
> +#  Philippe Mathieu-Daudé <philmd@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import sys
> +import os
> +import logging
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> +from avocado_qemu import Test
> +from qemu.binutils import binary_get_version
> +
> +
> +class PythonQemuCoreScripts(Test):
> +
> +    def test_get_version(self):
> +        logger = logging.getLogger('core')
> +        version = binary_get_version(self.qemu_bin)
> +        logger.debug('version: {}'.format(version))
> +        # QMP 'query-version' introduced with QEMU v0.14
> +        self.assertGreaterEqual(version['major'], 0)
> +        if version['major'] == 0:
> +            self.assertGreaterEqual(version['minor'], 14)