[4/5] scripts/qemu.py: introduce set_console() method

The set_console() method is intended to ease higher level use cases
that require a console device.

The amount of inteligence is limited on purpose, requiring either the
device type explicitly, or the existence of a machine (pattern)
definition.

Because of the console device type selection criteria (by machine
type), users should also be able to define that.  It'll then be used
for both '-machine' and for the console device type selection.

Users of the set_console() method will certainly be interested in
accessing the console device, and for that a console_socket property
has been added.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/qemu.py      |  97 +++++++++++++++++++++++-
 scripts/test_qemu.py | 176 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 scripts/test_qemu.py

On Thu, 05/24 20:58, Cleber Rosa wrote:
> The set_console() method is intended to ease higher level use cases
> that require a console device.
> 
> The amount of inteligence is limited on purpose, requiring either the
> device type explicitly, or the existence of a machine (pattern)
> definition.
> 
> Because of the console device type selection criteria (by machine
> type), users should also be able to define that.  It'll then be used
> for both '-machine' and for the console device type selection.
> 
> Users of the set_console() method will certainly be interested in
> accessing the console device, and for that a console_socket property
> has been added.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/qemu.py      |  97 +++++++++++++++++++++++-
>  scripts/test_qemu.py | 176 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 272 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/test_qemu.py
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 7813dd45ad..89db5bc6b2 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -17,19 +17,41 @@ import logging
>  import os
>  import subprocess
>  import qmp.qmp
> +import re
>  import shutil
> +import socket
>  import tempfile
>  
>  
>  LOG = logging.getLogger(__name__)
>  
>  
> +#: Maps machine types to the preferred console device types
> +CONSOLE_DEV_TYPES = {
> +    r'^clipper$': 'isa-serial',
> +    r'^malta': 'isa-serial',
> +    r'^(pc.*|q35.*|isapc)$': 'isa-serial',
> +    r'^(40p|powernv|prep)$': 'isa-serial',
> +    r'^pseries.*': 'spapr-vty',
> +    r'^s390-ccw-virtio.*': 'sclpconsole',
> +    }
> +
> +
>  class QEMUMachineError(Exception):
>      """
>      Exception called when an error in QEMUMachine happens.
>      """
>  
>  
> +class QEMUMachineAddDeviceError(QEMUMachineError):
> +    """
> +    Exception raised when a request to add a device can not be fulfilled
> +
> +    The failures are caused by limitations, lack of information or conflicting
> +    requests on the QEMUMachine methods.  This exception does not represent
> +    failures reported by the QEMU binary itself.
> +    """
> +
>  class MonitorResponseError(qmp.qmp.QMPError):
>      '''
>      Represents erroneous QMP monitor reply
> @@ -91,6 +113,10 @@ class QEMUMachine(object):
>          self._test_dir = test_dir
>          self._temp_dir = None
>          self._launched = False
> +        self._machine = None
> +        self._console_device_type = None
> +        self._console_address = None
> +        self._console_socket = None
>  
>          # just in case logging wasn't configured by the main script:
>          logging.basicConfig()
> @@ -175,9 +201,19 @@ class QEMUMachine(object):
>                  self._monitor_address[1])
>          else:
>              moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
> -        return ['-chardev', moncdev,
> +        args = ['-chardev', moncdev,
>                  '-mon', 'chardev=mon,mode=control',
>                  '-display', 'none', '-vga', 'none']
> +        if self._machine is not None:
> +            args.extend(['-machine', self._machine])
> +        if self._console_device_type is not None:
> +            self._console_address = os.path.join(self._temp_dir,
> +                                                 self._name + "-console.sock")
> +            chardev = ('socket,id=console,path=%s,server,nowait' %
> +                       self._console_address)
> +            device = '%s,chardev=console' % self._console_device_type
> +            args.extend(['-chardev', chardev, '-device', device])
> +        return args
>  
>      def _pre_launch(self):
>          self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> @@ -202,6 +238,10 @@ class QEMUMachine(object):
>  
>          self._qemu_log_path = None
>  
> +        if self._console_socket is not None:
> +            self._console_socket.close()
> +            self._console_socket = None
> +
>          if self._temp_dir is not None:
>              shutil.rmtree(self._temp_dir)
>              self._temp_dir = None
> @@ -365,3 +405,58 @@ class QEMUMachine(object):
>          Adds to the list of extra arguments to be given to the QEMU binary
>          '''
>          return self._args.extend(args)
> +
> +    def set_machine(self, machine_type):
> +        '''
> +        Sets the machine type
> +
> +        If set, the machine type will be added to the base arguments
> +        of the resulting QEMU command line.
> +        '''
> +        self._machine = machine_type
> +
> +    def set_console(self, device_type=None):
> +        '''
> +        Sets the device type for a console device
> +
> +        If set, the console device and a backing character device will
> +        be added to the base arguments of the resulting QEMU command
> +        line.
> +
> +        This is a convenience method that will either use the provided
> +        device type, of if not given, it will used the device type set
> +        on CONSOLE_DEV_TYPES.
> +
> +        The actual setting of command line arguments will be be done at
> +        machine launch time, as it depends on the temporary directory
> +        to be created.
> +
> +        @param device_type: the device type, such as "isa-serial"
> +        @raises: QEMUMachineAddDeviceError if the device type is not given
> +                 and can not be determined.
> +        '''
> +        if device_type is None:
> +            if self._machine is None:
> +                raise QEMUMachineAddDeviceError("Can not add a console device:"
> +                                                " QEMU instance without a "
> +                                                "defined machine type")
> +            for regex, device in CONSOLE_DEV_TYPES.items():
> +                if re.match(regex, self._machine):
> +                    device_type = device
> +                    break
> +            if device_type is None:
> +                raise QEMUMachineAddDeviceError("Can not add a console device:"
> +                                                " no matching console device "
> +                                                "type definition")
> +        self._console_device_type = device_type
> +
> +    @property
> +    def console_socket(self):
> +        """
> +        Returns a socket connected to the console
> +        """
> +        if self._console_socket is None:
> +            self._console_socket = socket.socket(socket.AF_UNIX,
> +                                                 socket.SOCK_STREAM)
> +            self._console_socket.connect(self._console_address)
> +        return self._console_socket
> diff --git a/scripts/test_qemu.py b/scripts/test_qemu.py
> new file mode 100644
> index 0000000000..5e016d3751
> --- /dev/null
> +++ b/scripts/test_qemu.py
> @@ -0,0 +1,176 @@

Again, a file header is missing.

> +import sys
> +import os
> +import glob
> +import unittest
> +import shutil
> +import tempfile
> +import subprocess
> +
> +import qemu
> +import qmp.qmp
> +
> +
> +class QEMUMachineProbeError(Exception):
> +    """
> +    Exception raised when a probe a fails to be deterministic
> +    """
> +
> +
> +def get_built_qemu_binaries(root_dir=None):
> +    """
> +    Attempts to find QEMU binaries in a tree
> +
> +    If root_dir is None, it will attempt to find the binaries at the
> +    source tree.  It's possible to override it by setting the environment
> +    variable QEMU_ROOT_DIR.
> +    """
> +    if root_dir is None:
> +        src_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
> +        root_dir = os.environ.get("QEMU_ROOT_DIR", src_dir)
> +    binaries = glob.glob(os.path.join(root_dir, '*-softmmu/qemu-system-*'))
> +    if 'win' in sys.platform:
> +        bin_filter = lambda x: x.endswith(".exe")
> +    else:
> +        bin_filter = lambda x: not x.endswith(".exe")
> +    return [_ for _ in binaries if bin_filter(_)]
> +
> +
> +def subprocess_dev_null(mode='w'):
> +    """
> +    A portable null file object suitable for use with the subprocess module
> +    """
> +    if hasattr(subprocess, 'DEVNULL'):
> +        return subprocess.DEVNULL
> +    else:
> +        return open(os.path.devnull, mode)
> +
> +
> +def qmp_execute(binary_path, qmp_command):
> +    """
> +    Executes a QMP command on a given QEMU binary
> +
> +    Useful for one-off execution of QEMU binaries to get runtime
> +    information.
> +
> +    @param binary_path: path to a QEMU binary
> +    @param qmp_command: the QMP command
> +    @note: passing arguments to the QMP command is not supported at
> +           this time.
> +    """
> +    try:
> +        tempdir = tempfile.mkdtemp()
> +        monitor_socket = os.path.join(tempdir, 'monitor.sock')
> +        args = [binary_path, '-nodefaults', '-machine', 'none',
> +                '-nographic', '-S', '-qmp', 'unix:%s' % monitor_socket]
> +        monitor = qmp.qmp.QEMUMonitorProtocol(monitor_socket, True)
> +        try:
> +            qemu_proc = subprocess.Popen(args,
> +                                         stdin=subprocess.PIPE,
> +                                         stdout=subprocess.PIPE,
> +                                         stderr=subprocess_dev_null(),
> +                                         universal_newlines=True)
> +        except OSError:
> +            return None
> +        monitor.accept()
> +        res = monitor.cmd(qmp_command)
> +        monitor.cmd("quit")
> +        qemu_proc.wait()
> +        monitor.close()
> +        return res.get("return", None)
> +    finally:
> +        shutil.rmtree(tempdir)
> +
> +
> +def qemu_bin_probe_arch(binary_path):
> +    """
> +    Probes the architecture from the QEMU binary
> +
> +    @returns: either the probed arch or None
> +    @rtype: str or None
> +    @raises: QEMUMachineProbeError
> +    """
> +    res = qmp_execute(binary_path, "query-target")
> +    if res is None:
> +        raise QEMUMachineProbeError('Failed to probe the QEMU arch by querying'
> +                                    ' the target of binary "%s"' % binary_path)
> +    return res.get("arch", None)
> +
> +
> +class QEMU(unittest.TestCase):

'QEMU' is too generic, what is the intended tests to do here? It seems to be
more about exercising the set_console() method.

Any test should be added to tests/, not scripts/.

> +
> +
> +    TEST_ARCH_MACHINE_CONSOLES = {
> +        'alpha': ['clipper'],
> +        'mips': ['malta'],
> +        'x86_64': ['isapc',
> +                   'pc', 'pc-0.10', 'pc-0.11', 'pc-0.12', 'pc-0.13',
> +                   'pc-0.14', 'pc-0.15', 'pc-1.0', 'pc-1.1', 'pc-1.2',
> +                   'pc-1.3',
> +                   'pc-i440fx-1.4', 'pc-i440fx-1.5', 'pc-i440fx-1.6',
> +                   'pc-i440fx-1.7', 'pc-i440fx-2.0', 'pc-i440fx-2.1',
> +                   'pc-i440fx-2.10', 'pc-i440fx-2.11', 'pc-i440fx-2.2',
> +                   'pc-i440fx-2.3', 'pc-i440fx-2.4', 'pc-i440fx-2.5',
> +                   'pc-i440fx-2.6', 'pc-i440fx-2.7', 'pc-i440fx-2.8',
> +                   'pc-i440fx-2.9', 'pc-q35-2.10', 'pc-q35-2.11',
> +                   'q35', 'pc-q35-2.4', 'pc-q35-2.5', 'pc-q35-2.6',
> +                   'pc-q35-2.7', 'pc-q35-2.8', 'pc-q35-2.9'],
> +        'ppc64': ['40p', 'powernv', 'prep', 'pseries', 'pseries-2.1',
> +                  'pseries-2.2', 'pseries-2.3', 'pseries-2.4', 'pseries-2.5',
> +                  'pseries-2.6', 'pseries-2.7', 'pseries-2.8', 'pseries-2.9',
> +                  'pseries-2.10', 'pseries-2.11', 'pseries-2.12'],
> +        's390x': ['s390-ccw-virtio', 's390-ccw-virtio-2.4',
> +                  's390-ccw-virtio-2.5', 's390-ccw-virtio-2.6',
> +                  's390-ccw-virtio-2.7', 's390-ccw-virtio-2.8',
> +                  's390-ccw-virtio-2.9', 's390-ccw-virtio-2.10',
> +                  's390-ccw-virtio-2.11', 's390-ccw-virtio-2.12']
> +    }
> +
> +
> +    def test_set_console(self):
> +        for machines in QEMU.TEST_ARCH_MACHINE_CONSOLES.values():
> +            for machine in machines:
> +                qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
> +                qemu_machine.set_machine(machine)
> +                qemu_machine.set_console()
> +
> +    def test_set_console_no_machine(self):
> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
> +                          qemu_machine.set_console)
> +
> +    def test_set_console_no_machine_match(self):
> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
> +        qemu_machine.set_machine('unknown-machine-model')
> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
> +                          qemu_machine.set_console)
> +
> +    @unittest.skipUnless(get_built_qemu_binaries(),
> +                         "Could not find any QEMU binaries built to use on "
> +                         "console check")
> +    def test_set_console_launch(self):
> +        for binary in get_built_qemu_binaries():
> +            probed_arch = qemu_bin_probe_arch(binary)
> +            for machine in QEMU.TEST_ARCH_MACHINE_CONSOLES.get(probed_arch, []):
> +                qemu_machine = qemu.QEMUMachine(binary)
> +
> +                # the following workarounds are target specific required for
> +                # this test.  users are of QEMUMachine are expected to deal with
> +                # target specific requirements just the same in their own code
> +                cap_htm_off = ('pseries-2.7', 'pseries-2.8', 'pseries-2.9',
> +                               'pseries-2.10', 'pseries-2.11', 'pseries-2.12')
> +                if probed_arch == 'ppc64' and machine in cap_htm_off:
> +                    qemu_machine._machine = machine   # pylint: disable=W0212
> +                    qemu_machine.args.extend(['-machine',
> +                                              '%s,cap-htm=off' % machine])
> +                elif probed_arch == 's390x':
> +                    qemu_machine.set_machine(machine)
> +                    qemu_machine.args.append('-nodefaults')
> +                elif probed_arch == 'mips':
> +                    qemu_machine.set_machine(machine)
> +                    qemu_machine.args.extend(['-bios', os.path.devnull])
> +                else:
> +                    qemu_machine.set_machine(machine)
> +
> +                qemu_machine.set_console()
> +                qemu_machine.launch()
> +                qemu_machine.shutdown()
> -- 
> 2.17.0
> 

Fam

On 05/25/2018 01:47 AM, Fam Zheng wrote:
> On Thu, 05/24 20:58, Cleber Rosa wrote:
>> The set_console() method is intended to ease higher level use cases
>> that require a console device.
>>
>> The amount of inteligence is limited on purpose, requiring either the
>> device type explicitly, or the existence of a machine (pattern)
>> definition.
>>

Typo here (s/inteligence/intelligence").

>> Because of the console device type selection criteria (by machine
>> type), users should also be able to define that.  It'll then be used
>> for both '-machine' and for the console device type selection.
>>
>> Users of the set_console() method will certainly be interested in
>> accessing the console device, and for that a console_socket property
>> has been added.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>  scripts/qemu.py      |  97 +++++++++++++++++++++++-
>>  scripts/test_qemu.py | 176 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 272 insertions(+), 1 deletion(-)
>>  create mode 100644 scripts/test_qemu.py
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 7813dd45ad..89db5bc6b2 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -17,19 +17,41 @@ import logging
>>  import os
>>  import subprocess
>>  import qmp.qmp
>> +import re
>>  import shutil
>> +import socket
>>  import tempfile
>>  
>>  
>>  LOG = logging.getLogger(__name__)
>>  
>>  
>> +#: Maps machine types to the preferred console device types
>> +CONSOLE_DEV_TYPES = {
>> +    r'^clipper$': 'isa-serial',
>> +    r'^malta': 'isa-serial',
>> +    r'^(pc.*|q35.*|isapc)$': 'isa-serial',
>> +    r'^(40p|powernv|prep)$': 'isa-serial',
>> +    r'^pseries.*': 'spapr-vty',
>> +    r'^s390-ccw-virtio.*': 'sclpconsole',
>> +    }
>> +
>> +
>>  class QEMUMachineError(Exception):
>>      """
>>      Exception called when an error in QEMUMachine happens.
>>      """
>>  
>>  
>> +class QEMUMachineAddDeviceError(QEMUMachineError):
>> +    """
>> +    Exception raised when a request to add a device can not be fulfilled
>> +
>> +    The failures are caused by limitations, lack of information or conflicting
>> +    requests on the QEMUMachine methods.  This exception does not represent
>> +    failures reported by the QEMU binary itself.
>> +    """
>> +
>>  class MonitorResponseError(qmp.qmp.QMPError):
>>      '''
>>      Represents erroneous QMP monitor reply
>> @@ -91,6 +113,10 @@ class QEMUMachine(object):
>>          self._test_dir = test_dir
>>          self._temp_dir = None
>>          self._launched = False
>> +        self._machine = None
>> +        self._console_device_type = None
>> +        self._console_address = None
>> +        self._console_socket = None
>>  
>>          # just in case logging wasn't configured by the main script:
>>          logging.basicConfig()
>> @@ -175,9 +201,19 @@ class QEMUMachine(object):
>>                  self._monitor_address[1])
>>          else:
>>              moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
>> -        return ['-chardev', moncdev,
>> +        args = ['-chardev', moncdev,
>>                  '-mon', 'chardev=mon,mode=control',
>>                  '-display', 'none', '-vga', 'none']
>> +        if self._machine is not None:
>> +            args.extend(['-machine', self._machine])
>> +        if self._console_device_type is not None:
>> +            self._console_address = os.path.join(self._temp_dir,
>> +                                                 self._name + "-console.sock")
>> +            chardev = ('socket,id=console,path=%s,server,nowait' %
>> +                       self._console_address)
>> +            device = '%s,chardev=console' % self._console_device_type
>> +            args.extend(['-chardev', chardev, '-device', device])
>> +        return args
>>  
>>      def _pre_launch(self):
>>          self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
>> @@ -202,6 +238,10 @@ class QEMUMachine(object):
>>  
>>          self._qemu_log_path = None
>>  
>> +        if self._console_socket is not None:
>> +            self._console_socket.close()
>> +            self._console_socket = None
>> +
>>          if self._temp_dir is not None:
>>              shutil.rmtree(self._temp_dir)
>>              self._temp_dir = None
>> @@ -365,3 +405,58 @@ class QEMUMachine(object):
>>          Adds to the list of extra arguments to be given to the QEMU binary
>>          '''
>>          return self._args.extend(args)
>> +
>> +    def set_machine(self, machine_type):
>> +        '''
>> +        Sets the machine type
>> +
>> +        If set, the machine type will be added to the base arguments
>> +        of the resulting QEMU command line.
>> +        '''
>> +        self._machine = machine_type
>> +
>> +    def set_console(self, device_type=None):
>> +        '''
>> +        Sets the device type for a console device
>> +
>> +        If set, the console device and a backing character device will
>> +        be added to the base arguments of the resulting QEMU command
>> +        line.
>> +
>> +        This is a convenience method that will either use the provided
>> +        device type, of if not given, it will used the device type set
>> +        on CONSOLE_DEV_TYPES.
>> +
>> +        The actual setting of command line arguments will be be done at
>> +        machine launch time, as it depends on the temporary directory
>> +        to be created.
>> +
>> +        @param device_type: the device type, such as "isa-serial"
>> +        @raises: QEMUMachineAddDeviceError if the device type is not given
>> +                 and can not be determined.
>> +        '''
>> +        if device_type is None:
>> +            if self._machine is None:
>> +                raise QEMUMachineAddDeviceError("Can not add a console device:"
>> +                                                " QEMU instance without a "
>> +                                                "defined machine type")
>> +            for regex, device in CONSOLE_DEV_TYPES.items():
>> +                if re.match(regex, self._machine):
>> +                    device_type = device
>> +                    break
>> +            if device_type is None:
>> +                raise QEMUMachineAddDeviceError("Can not add a console device:"
>> +                                                " no matching console device "
>> +                                                "type definition")
>> +        self._console_device_type = device_type
>> +
>> +    @property
>> +    def console_socket(self):
>> +        """
>> +        Returns a socket connected to the console
>> +        """
>> +        if self._console_socket is None:
>> +            self._console_socket = socket.socket(socket.AF_UNIX,
>> +                                                 socket.SOCK_STREAM)
>> +            self._console_socket.connect(self._console_address)
>> +        return self._console_socket
>> diff --git a/scripts/test_qemu.py b/scripts/test_qemu.py
>> new file mode 100644
>> index 0000000000..5e016d3751
>> --- /dev/null
>> +++ b/scripts/test_qemu.py
>> @@ -0,0 +1,176 @@
> 
> Again, a file header is missing.
> 

Yep, adding one.

>> +import sys
>> +import os
>> +import glob
>> +import unittest
>> +import shutil
>> +import tempfile
>> +import subprocess
>> +
>> +import qemu
>> +import qmp.qmp
>> +
>> +
>> +class QEMUMachineProbeError(Exception):
>> +    """
>> +    Exception raised when a probe a fails to be deterministic
>> +    """
>> +
>> +
>> +def get_built_qemu_binaries(root_dir=None):
>> +    """
>> +    Attempts to find QEMU binaries in a tree
>> +
>> +    If root_dir is None, it will attempt to find the binaries at the
>> +    source tree.  It's possible to override it by setting the environment
>> +    variable QEMU_ROOT_DIR.
>> +    """
>> +    if root_dir is None:
>> +        src_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
>> +        root_dir = os.environ.get("QEMU_ROOT_DIR", src_dir)
>> +    binaries = glob.glob(os.path.join(root_dir, '*-softmmu/qemu-system-*'))
>> +    if 'win' in sys.platform:
>> +        bin_filter = lambda x: x.endswith(".exe")
>> +    else:
>> +        bin_filter = lambda x: not x.endswith(".exe")
>> +    return [_ for _ in binaries if bin_filter(_)]
>> +
>> +
>> +def subprocess_dev_null(mode='w'):
>> +    """
>> +    A portable null file object suitable for use with the subprocess module
>> +    """
>> +    if hasattr(subprocess, 'DEVNULL'):
>> +        return subprocess.DEVNULL
>> +    else:
>> +        return open(os.path.devnull, mode)
>> +
>> +
>> +def qmp_execute(binary_path, qmp_command):
>> +    """
>> +    Executes a QMP command on a given QEMU binary
>> +
>> +    Useful for one-off execution of QEMU binaries to get runtime
>> +    information.
>> +
>> +    @param binary_path: path to a QEMU binary
>> +    @param qmp_command: the QMP command
>> +    @note: passing arguments to the QMP command is not supported at
>> +           this time.
>> +    """
>> +    try:
>> +        tempdir = tempfile.mkdtemp()
>> +        monitor_socket = os.path.join(tempdir, 'monitor.sock')
>> +        args = [binary_path, '-nodefaults', '-machine', 'none',
>> +                '-nographic', '-S', '-qmp', 'unix:%s' % monitor_socket]
>> +        monitor = qmp.qmp.QEMUMonitorProtocol(monitor_socket, True)
>> +        try:
>> +            qemu_proc = subprocess.Popen(args,
>> +                                         stdin=subprocess.PIPE,
>> +                                         stdout=subprocess.PIPE,
>> +                                         stderr=subprocess_dev_null(),
>> +                                         universal_newlines=True)
>> +        except OSError:
>> +            return None
>> +        monitor.accept()
>> +        res = monitor.cmd(qmp_command)
>> +        monitor.cmd("quit")
>> +        qemu_proc.wait()
>> +        monitor.close()
>> +        return res.get("return", None)
>> +    finally:
>> +        shutil.rmtree(tempdir)
>> +
>> +
>> +def qemu_bin_probe_arch(binary_path):
>> +    """
>> +    Probes the architecture from the QEMU binary
>> +
>> +    @returns: either the probed arch or None
>> +    @rtype: str or None
>> +    @raises: QEMUMachineProbeError
>> +    """
>> +    res = qmp_execute(binary_path, "query-target")
>> +    if res is None:
>> +        raise QEMUMachineProbeError('Failed to probe the QEMU arch by querying'
>> +                                    ' the target of binary "%s"' % binary_path)
>> +    return res.get("arch", None)
>> +
>> +
>> +class QEMU(unittest.TestCase):
> 
> 'QEMU' is too generic, what is the intended tests to do here? It seems to be
> more about exercising the set_console() method.
> 

Yes, but again, I'm favoring simpler names when the scope is supposed to
be limited.  The chances of a name clash here are close to none IMO.  I
don't think this class would be reused elsewhere, or a class with the
same name would be imported here.

> Any test should be added to tests/, not scripts/.
> 

One of the reasons for this file to be in this patch was to generate
this exact discussion.  "qemu.py" itself is not a script, but a
module/library.  Still it lacks the structure to have accompanying tests.

I was hoping to get away with these tests here, so that they wouldn't be
thrown away, until "qemu.py", "qmp/*.py" and others are given the status
and structure of Python modules.

I can definitely move these to tests/, but how about its relation to the
other tests living there and its activation?  Should we recognize its
existence and add a check-* target?

- Cleber.

>> +
>> +
>> +    TEST_ARCH_MACHINE_CONSOLES = {
>> +        'alpha': ['clipper'],
>> +        'mips': ['malta'],
>> +        'x86_64': ['isapc',
>> +                   'pc', 'pc-0.10', 'pc-0.11', 'pc-0.12', 'pc-0.13',
>> +                   'pc-0.14', 'pc-0.15', 'pc-1.0', 'pc-1.1', 'pc-1.2',
>> +                   'pc-1.3',
>> +                   'pc-i440fx-1.4', 'pc-i440fx-1.5', 'pc-i440fx-1.6',
>> +                   'pc-i440fx-1.7', 'pc-i440fx-2.0', 'pc-i440fx-2.1',
>> +                   'pc-i440fx-2.10', 'pc-i440fx-2.11', 'pc-i440fx-2.2',
>> +                   'pc-i440fx-2.3', 'pc-i440fx-2.4', 'pc-i440fx-2.5',
>> +                   'pc-i440fx-2.6', 'pc-i440fx-2.7', 'pc-i440fx-2.8',
>> +                   'pc-i440fx-2.9', 'pc-q35-2.10', 'pc-q35-2.11',
>> +                   'q35', 'pc-q35-2.4', 'pc-q35-2.5', 'pc-q35-2.6',
>> +                   'pc-q35-2.7', 'pc-q35-2.8', 'pc-q35-2.9'],
>> +        'ppc64': ['40p', 'powernv', 'prep', 'pseries', 'pseries-2.1',
>> +                  'pseries-2.2', 'pseries-2.3', 'pseries-2.4', 'pseries-2.5',
>> +                  'pseries-2.6', 'pseries-2.7', 'pseries-2.8', 'pseries-2.9',
>> +                  'pseries-2.10', 'pseries-2.11', 'pseries-2.12'],
>> +        's390x': ['s390-ccw-virtio', 's390-ccw-virtio-2.4',
>> +                  's390-ccw-virtio-2.5', 's390-ccw-virtio-2.6',
>> +                  's390-ccw-virtio-2.7', 's390-ccw-virtio-2.8',
>> +                  's390-ccw-virtio-2.9', 's390-ccw-virtio-2.10',
>> +                  's390-ccw-virtio-2.11', 's390-ccw-virtio-2.12']
>> +    }
>> +
>> +
>> +    def test_set_console(self):
>> +        for machines in QEMU.TEST_ARCH_MACHINE_CONSOLES.values():
>> +            for machine in machines:
>> +                qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
>> +                qemu_machine.set_machine(machine)
>> +                qemu_machine.set_console()
>> +
>> +    def test_set_console_no_machine(self):
>> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
>> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
>> +                          qemu_machine.set_console)
>> +
>> +    def test_set_console_no_machine_match(self):
>> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
>> +        qemu_machine.set_machine('unknown-machine-model')
>> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
>> +                          qemu_machine.set_console)
>> +
>> +    @unittest.skipUnless(get_built_qemu_binaries(),
>> +                         "Could not find any QEMU binaries built to use on "
>> +                         "console check")
>> +    def test_set_console_launch(self):
>> +        for binary in get_built_qemu_binaries():
>> +            probed_arch = qemu_bin_probe_arch(binary)
>> +            for machine in QEMU.TEST_ARCH_MACHINE_CONSOLES.get(probed_arch, []):
>> +                qemu_machine = qemu.QEMUMachine(binary)
>> +
>> +                # the following workarounds are target specific required for
>> +                # this test.  users are of QEMUMachine are expected to deal with
>> +                # target specific requirements just the same in their own code
>> +                cap_htm_off = ('pseries-2.7', 'pseries-2.8', 'pseries-2.9',
>> +                               'pseries-2.10', 'pseries-2.11', 'pseries-2.12')
>> +                if probed_arch == 'ppc64' and machine in cap_htm_off:
>> +                    qemu_machine._machine = machine   # pylint: disable=W0212
>> +                    qemu_machine.args.extend(['-machine',
>> +                                              '%s,cap-htm=off' % machine])
>> +                elif probed_arch == 's390x':
>> +                    qemu_machine.set_machine(machine)
>> +                    qemu_machine.args.append('-nodefaults')
>> +                elif probed_arch == 'mips':
>> +                    qemu_machine.set_machine(machine)
>> +                    qemu_machine.args.extend(['-bios', os.path.devnull])
>> +                else:
>> +                    qemu_machine.set_machine(machine)
>> +
>> +                qemu_machine.set_console()
>> +                qemu_machine.launch()
>> +                qemu_machine.shutdown()
>> -- 
>> 2.17.0
>>
> 
> Fam
>

On Fri, May 25, 2018 at 12:57:54PM -0400, Cleber Rosa wrote:
> On 05/25/2018 01:47 AM, Fam Zheng wrote:
[...]
> >> +class QEMU(unittest.TestCase):
> > 
> > 'QEMU' is too generic, what is the intended tests to do here? It seems to be
> > more about exercising the set_console() method.
> > 
> 
> Yes, but again, I'm favoring simpler names when the scope is supposed to
> be limited.  The chances of a name clash here are close to none IMO.  I
> don't think this class would be reused elsewhere, or a class with the
> same name would be imported here.
> 
> > Any test should be added to tests/, not scripts/.
> > 
> 
> One of the reasons for this file to be in this patch was to generate
> this exact discussion.  "qemu.py" itself is not a script, but a
> module/library.  Still it lacks the structure to have accompanying tests.
> 
> I was hoping to get away with these tests here, so that they wouldn't be
> thrown away, until "qemu.py", "qmp/*.py" and others are given the status
> and structure of Python modules.
> 
> I can definitely move these to tests/, but how about its relation to the
> other tests living there and its activation?  Should we recognize its
> existence and add a check-* target?

"scripts/test_qemu.py" makes it look like a script that will test
QEMU.  While we don't rearrange the Python modules to follow a
more Pythonic structure, probably moving it to tests/ is the best
option we have.  Maybe tests/python?

Wherever we store the test code, running those tests on
"make check" is a good idea.

But I wouldn't like this to delay the inclusion of this series.
If rearranging the Python modules would make the job easier,
including the test code only later would be a reasonable option,
too.

On 05/29/2018 03:06 PM, Eduardo Habkost wrote:
> On Fri, May 25, 2018 at 12:57:54PM -0400, Cleber Rosa wrote:
>> On 05/25/2018 01:47 AM, Fam Zheng wrote:
> [...]
>>>> +class QEMU(unittest.TestCase):
>>>
>>> 'QEMU' is too generic, what is the intended tests to do here? It seems to be
>>> more about exercising the set_console() method.
>>>
>>
>> Yes, but again, I'm favoring simpler names when the scope is supposed to
>> be limited.  The chances of a name clash here are close to none IMO.  I
>> don't think this class would be reused elsewhere, or a class with the
>> same name would be imported here.
>>
>>> Any test should be added to tests/, not scripts/.
>>>
>>
>> One of the reasons for this file to be in this patch was to generate
>> this exact discussion.  "qemu.py" itself is not a script, but a
>> module/library.  Still it lacks the structure to have accompanying tests.
>>
>> I was hoping to get away with these tests here, so that they wouldn't be
>> thrown away, until "qemu.py", "qmp/*.py" and others are given the status
>> and structure of Python modules.
>>
>> I can definitely move these to tests/, but how about its relation to the
>> other tests living there and its activation?  Should we recognize its
>> existence and add a check-* target?
> 
> "scripts/test_qemu.py" makes it look like a script that will test
> QEMU.  While we don't rearrange the Python modules to follow a
> more Pythonic structure, probably moving it to tests/ is the best
> option we have.  Maybe tests/python?
> 
> Wherever we store the test code, running those tests on
> "make check" is a good idea.
> 
> But I wouldn't like this to delay the inclusion of this series.
> If rearranging the Python modules would make the job easier,
> including the test code only later would be a reasonable option,
> too.
> 

We already have a "probable next steps" planning (see PATCH 0), and we
can include a "Python module reorganization" to it.  So, I'm dropping
this test on v3 and we'll add it later, at a better location.

- Cleber.