Re: [Qemu-devel] [PULL 4/7] scripts: refactor the VM class in iotests for reuse

[Max Reitz]

On 22.07.2016 10:00, Amit Shah wrote:
> From: "Daniel P. Berrange" <address@hidden>
> 
> The iotests module has a python class for controlling QEMU
> processes. Pull the generic functionality out of this file
> and create a scripts/qemu.py module containing a QEMUMachine
> class. Put the QTest integration support into a subclass
> QEMUQtestMachine.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Amit Shah <address@hidden>
> ---
>  scripts/qemu.py               | 202 
> ++++++++++++++++++++++++++++++++++++++++++
>  scripts/qtest.py              |  34 +++++++
>  tests/qemu-iotests/iotests.py | 135 +---------------------------
>  3 files changed, 240 insertions(+), 131 deletions(-)
>  create mode 100644 scripts/qemu.py

Hm, I have even more questions...

(I'm starting to wonder if I'm doing anything horribly wrong, because
this patch basically completely breaks the Python iotests for me.)

> diff --git a/scripts/qemu.py b/scripts/qemu.py
> new file mode 100644
> index 0000000..9cdad24
> --- /dev/null
> +++ b/scripts/qemu.py
> @@ -0,0 +1,202 @@
> +# QEMU library
> +#
> +# Copyright (C) 2015-2016 Red Hat Inc.
> +# Copyright (C) 2012 IBM Corp.
> +#
> +# Authors:
> +#  Fam Zheng <address@hidden>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +# Based on qmp.py.
> +#
> +
> +import errno
> +import string
> +import os
> +import sys
> +import subprocess
> +import qmp.qmp
> +
> +
> +class QEMUMachine(object):
> +    '''A QEMU VM'''
> +
> +    def __init__(self, binary, args=[], wrapper=[], name=None, 
> test_dir="/var/tmp",
> +                 monitor_address=None, debug=False):
> +        if name is None:
> +            name = "qemu-%d" % os.getpid()
> +        if monitor_address is None:
> +            monitor_address = os.path.join(test_dir, name + "-monitor.sock")
> +        self._monitor_address = monitor_address
> +        self._qemu_log_path = os.path.join(test_dir, name + ".log")
> +        self._popen = None
> +        self._binary = binary
> +        self._args = args
> +        self._wrapper = wrapper
> +        self._events = []
> +        self._iolog = None
> +        self._debug = debug
> +
> +    # This can be used to add an unused monitor instance.
> +    def add_monitor_telnet(self, ip, port):
> +        args = 'tcp:%s:%d,server,nowait,telnet' % (ip, port)
> +        self._args.append('-monitor')
> +        self._args.append(args)
> +
> +    def add_fd(self, fd, fdset, opaque, opts=''):
> +        '''Pass a file descriptor to the VM'''
> +        options = ['fd=%d' % fd,
> +                   'set=%d' % fdset,
> +                   'opaque=%s' % opaque]
> +        if opts:
> +            options.append(opts)
> +
> +        self._args.append('-add-fd')
> +        self._args.append(','.join(options))
> +        return self
> +
> +    def send_fd_scm(self, fd_file_path):
> +        # In iotest.py, the qmp should always use unix socket.
> +        assert self._qmp.is_scm_available()
> +        bin = socket_scm_helper
> +        if os.path.exists(bin) == False:
> +            print "Scm help program does not present, path '%s'." % bin
> +            return -1
> +        fd_param = ["%s" % bin,
> +                    "%d" % self._qmp.get_sock_fd(),
> +                    "%s" % fd_file_path]
> +        devnull = open('/dev/null', 'rb')
> +        p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
> +                             stderr=sys.stderr)
> +        return p.wait()
> +
> +    @staticmethod
> +    def _remove_if_exists(path):
> +        '''Remove file object at path if it exists'''
> +        try:
> +            os.remove(path)
> +        except OSError as exception:
> +            if exception.errno == errno.ENOENT:
> +                return
> +            raise
> +
> +    def get_pid(self):
> +        if not self._popen:
> +            return None
> +        return self._popen.pid
> +
> +    def _load_io_log(self):
> +        with open(self._qemu_log_path, "r") as fh:
> +            self._iolog = fh.read()
> +
> +    def _base_args(self):
> +        if isinstance(self._monitor_address, tuple):
> +            moncdev = "socket,id=mon,host=%s,port=%s" % (
> +                self._monitor_address[0],
> +                self._monitor_address[1])
> +        else:
> +            moncdev = 'socket,id=mon,path=%s' % self._monitor_address
> +        return ['-chardev', moncdev,
> +                '-mon', 'chardev=mon,mode=control',
> +                '-display', 'none', '-vga', 'none']
> +
> +    def _pre_launch(self):
> +        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, 
> server=True,
> +                                                debug=self._debug)
> +
> +    def _post_launch(self):
> +        self._qmp.accept()
> +
> +    def _post_shutdown(self):
> +        if not isinstance(self._monitor_address, tuple):
> +            self._remove_if_exists(self._monitor_address)
> +        self._remove_if_exists(self._qemu_log_path)
> +
> +    def launch(self):
> +        '''Launch the VM and establish a QMP connection'''
> +        devnull = open('/dev/null', 'rb')
> +        qemulog = open(self._qemu_log_path, 'wb')
> +        try:
> +            self._pre_launch()
> +            args = self._wrapper + [self._binary] + self._base_args() + 
> self._args

self._binary is already an array. This is because it's taken directly
from... [1]

The superfluous array brackets result in:

AttributeError: 'list' object has no attribute 'rfind'

> +            self._popen = subprocess.Popen(args, stdin=devnull, 
> stdout=qemulog,
> +                                           stderr=subprocess.STDOUT, 
> shell=False)
> +            self._post_launch()
> +        except:
> +            if self._popen:
> +                self._popen.kill()
> +            self._load_io_log()
> +            self._post_shutdown()
> +            self._popen = None
> +            raise
> +
> +    def shutdown(self):
> +        '''Terminate the VM and clean up'''
> +        if not self._popen is None:
> +            try:
> +                self._qmp.cmd('quit')
> +                self._qmp.close()
> +            except:
> +                self._popen.kill()
> +
> +            exitcode = self._popen.wait()
> +            if exitcode < 0:
> +                sys.stderr.write('qemu received signal %i: %s\n' % 
> (-exitcode, ' '.join(self._args)))
> +            self._load_io_log()
> +            self._post_shutdown()
> +            self._popen = None
> +
> +    underscore_to_dash = string.maketrans('_', '-')
> +    def qmp(self, cmd, conv_keys=True, **args):
> +        '''Invoke a QMP command and return the result dict'''
> +        qmp_args = dict()
> +        for k in args.keys():
> +            if conv_keys:
> +                qmp_args[k.translate(self.underscore_to_dash)] = args[k]
> +            else:
> +                qmp_args[k] = args[k]
> +
> +        return self._qmp.cmd(cmd, args=qmp_args)
> +
> +    def command(self, cmd, conv_keys=True, **args):
> +        reply = self.qmp(cmd, conv_keys, **args)
> +        if reply is None:
> +            raise Exception("Monitor is closed")
> +        if "error" in reply:
> +            raise Exception(reply["error"]["desc"])
> +        return reply["return"]
> +
> +    def get_qmp_event(self, wait=False):
> +        '''Poll for one queued QMP events and return it'''
> +        if len(self._events) > 0:
> +            return self._events.pop(0)
> +        return self._qmp.pull_event(wait=wait)
> +
> +    def get_qmp_events(self, wait=False):
> +        '''Poll for queued QMP events and return a list of dicts'''
> +        events = self._qmp.get_events(wait=wait)
> +        events.extend(self._events)
> +        del self._events[:]
> +        self._qmp.clear_events()
> +        return events
> +
> +    def event_wait(self, name, timeout=60.0, match=None):
> +        # Search cached events
> +        for event in self._events:
> +            if (event['event'] == name) and event_match(event, match):
> +                self._events.remove(event)
> +                return event
> +
> +        # Poll for new events
> +        while True:
> +            event = self._qmp.pull_event(wait=timeout)
> +            if (event['event'] == name) and event_match(event, match):

event_match is not defined in this file.

> +                return event
> +            self._events.append(event)
> +
> +        return None
> +
> +    def get_log(self):
> +        return self._iolog
> diff --git a/scripts/qtest.py b/scripts/qtest.py
> index a971445..03bc7f6 100644
> --- a/scripts/qtest.py
> +++ b/scripts/qtest.py
> @@ -13,6 +13,11 @@
>  
>  import errno
>  import socket
> +import string
> +import os
> +import subprocess
> +import qmp.qmp
> +import qemu
>  
>  class QEMUQtestProtocol(object):
>      def __init__(self, address, server=False):
> @@ -69,3 +74,32 @@ class QEMUQtestProtocol(object):
>  
>      def settimeout(self, timeout):
>          self._sock.settimeout(timeout)
> +
> +
> +class QEMUQtestMachine(qemu.QEMUMachine):
> +    '''A QEMU VM'''
> +
> +    def __init__(self, binary, args=[], name=None, test_dir="/var/tmp"):
> +        super(self, QEMUQtestMachine).__init__(binary, args, name, test_dir)

You're passing name as wrapper. Probably wrong.

Also, shouldn't the arguments to super() be the other way around? I.e.
super(QEMUQtestMachine, self)? On one hand, this is what Stack Overflow
tells me, and it also makes Python stop complaining when running any
Python iotest.

> +        self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")

What if name is None?

> +
> +    def _base_args(self):
> +        args = super(self, QEMUQtestMachine)._base_args()
> +        args.extend(['-qtest', 'unix:path=' + self._qtest_path])
> +        return args
> +
> +    def _pre_launch(self):
> +        super(self, QEMUQtestMachine)._pre_launch()
> +        self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
> +
> +    def _post_launch(self):
> +        super(self, QEMUQtestMachine)._post_launch()
> +        self._qtest.accept()
> +
> +    def _post_shutdown(self):
> +        super(self, QEMUQtestMachine)._post_shutdown()
> +        self._remove_if_exists(self._qtest_path)
> +
> +    def qtest(self, cmd):
> +        '''Send a qtest command to guest'''
> +        return self._qtest.cmd(cmd)
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 1687c33..14427f4 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -24,8 +24,6 @@ import string
>  import unittest
>  import sys
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 
> 'scripts'))
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 
> 'scripts', 'qmp'))
> -import qmp
>  import qtest
>  import struct
>  import json
> @@ -41,9 +39,8 @@ qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
>  if os.environ.get('QEMU_IO_OPTIONS'):
>      qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
>  
> -qemu_args = [os.environ.get('QEMU_PROG', 'qemu')]
> -if os.environ.get('QEMU_OPTIONS'):
> -    qemu_args += os.environ['QEMU_OPTIONS'].strip().split(' ')
> +qemu_prog = [os.environ.get('QEMU_PROG', 'qemu')]
> +qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>  
>  imgfmt = os.environ.get('IMGFMT', 'raw')
>  imgproto = os.environ.get('IMGPROTO', 'file')
> @@ -148,27 +145,12 @@ def event_match(event, match=None):
>  
>      return True
>  
> -class VM(object):
> +class VM(qtest.QEMUMachine):
>      '''A QEMU VM'''
>  
>      def __init__(self):
> -        self._monitor_path = os.path.join(test_dir, 'qemu-mon.%d' % 
> os.getpid())
> -        self._qemu_log_path = os.path.join(test_dir, 'qemu-log.%d' % 
> os.getpid())
> -        self._qtest_path = os.path.join(test_dir, 'qemu-qtest.%d' % 
> os.getpid())
> -        self._args = qemu_args + ['-chardev',
> -                     'socket,id=mon,path=' + self._monitor_path,
> -                     '-mon', 'chardev=mon,mode=control',
> -                     '-qtest', 'unix:path=' + self._qtest_path,
> -                     '-machine', 'accel=qtest',
> -                     '-display', 'none', '-vga', 'none']
> +        super(self, VM).__init__(qemu_prog, qemu_opts, test_dir)

Same super() issue as above.

Also, if this is indeed supposed to subclass qtest.QEMUQtestMachine,
then you are passing test_dir as name, which would be wrong.

[1] ...here. qemu_prog is an array.

>          self._num_drives = 0
> -        self._events = []
> -
> -    # This can be used to add an unused monitor instance.
> -    def add_monitor_telnet(self, ip, port):
> -        args = 'tcp:%s:%d,server,nowait,telnet' % (ip, port)
> -        self._args.append('-monitor')
> -        self._args.append(args)
>  
>      def add_drive_raw(self, opts):
>          self._args.append('-drive')

[...]

But even after working around all of this, I'm still getting a socket
timeout which I can't really figure out why. All I know is that the qemu
process doesn't seem to get started really well:

$ ps -e | grep qemu
19255 pts/5    00:00:00 qemu-system-x86 <defunct>

The stdout log however is empty and there's no useful output in the
iotest itself.

Max

signature.asc
Description: OpenPGP digital signature