From 2d853c70a25a2c4b812eadd304c9e35882eb2e0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:04 +0200 Subject: [PATCH 01/15] qemu.py: Pylint/style fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No actual code changes, just several pylint/style fixes and docstring clarifications. Signed-off-by: Lukáš Doktor Reviewed-by: Stefan Hajnoczi Message-Id: <20170818142613.32394-2-ldoktor@redhat.com> Reviewed-by: Cleber Rosa Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 73 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 4d8ee10943..b45e691538 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -30,8 +30,22 @@ class QEMUMachine(object): # vm is guaranteed to be shut down here ''' - def __init__(self, binary, args=[], wrapper=[], name=None, test_dir="/var/tmp", - monitor_address=None, socket_scm_helper=None, debug=False): + def __init__(self, binary, args=[], wrapper=[], name=None, + test_dir="/var/tmp", monitor_address=None, + socket_scm_helper=None, debug=False): + ''' + Initialize a QEMUMachine + + @param binary: path to the qemu binary + @param args: list of extra arguments + @param wrapper: list of arguments used as prefix to qemu binary + @param name: prefix for socket and log file names (default: qemu-PID) + @param test_dir: where to create socket and log file + @param monitor_address: address for QMP monitor + @param socket_scm_helper: helper program, required for send_fd_scm()" + @param debug: enable debug mode + @note: Qemu process is not started until launch() is used. + ''' if name is None: name = "qemu-%d" % os.getpid() if monitor_address is None: @@ -40,12 +54,13 @@ class QEMUMachine(object): self._qemu_log_path = os.path.join(test_dir, name + ".log") self._popen = None self._binary = binary - self._args = list(args) # Force copy args in case we modify them + self._args = list(args) # Force copy args in case we modify them self._wrapper = wrapper self._events = [] self._iolog = None self._socket_scm_helper = socket_scm_helper self._debug = debug + self._qmp = None def __enter__(self): return self @@ -78,16 +93,16 @@ class QEMUMachine(object): if self._socket_scm_helper is None: print >>sys.stderr, "No path to socket_scm_helper set" return -1 - if os.path.exists(self._socket_scm_helper) == False: + if not os.path.exists(self._socket_scm_helper): print >>sys.stderr, "%s does not exist" % self._socket_scm_helper return -1 fd_param = ["%s" % self._socket_scm_helper, "%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() + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout, + stderr=sys.stderr) + return proc.wait() @staticmethod def _remove_if_exists(path): @@ -113,8 +128,8 @@ class QEMUMachine(object): return self._popen.pid def _load_io_log(self): - with open(self._qemu_log_path, "r") as fh: - self._iolog = fh.read() + with open(self._qemu_log_path, "r") as iolog: + self._iolog = iolog.read() def _base_args(self): if isinstance(self._monitor_address, tuple): @@ -128,7 +143,8 @@ class QEMUMachine(object): '-display', 'none', '-vga', 'none'] def _pre_launch(self): - self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True, + self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, + server=True, debug=self._debug) def _post_launch(self): @@ -145,9 +161,11 @@ class QEMUMachine(object): qemulog = open(self._qemu_log_path, 'wb') try: self._pre_launch() - args = self._wrapper + [self._binary] + self._base_args() + self._args + args = (self._wrapper + [self._binary] + self._base_args() + + self._args) self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog, - stderr=subprocess.STDOUT, shell=False) + stderr=subprocess.STDOUT, + shell=False) self._post_launch() except: if self.is_running(): @@ -168,23 +186,30 @@ class QEMUMachine(object): exitcode = self._popen.wait() if exitcode < 0: - sys.stderr.write('qemu received signal %i: %s\n' % (-exitcode, ' '.join(self._args))) + sys.stderr.write('qemu received signal %i: %s\n' + % (-exitcode, ' '.join(self._args))) self._load_io_log() self._post_shutdown() underscore_to_dash = string.maketrans('_', '-') + def qmp(self, cmd, conv_keys=True, **args): - '''Invoke a QMP command and return the result dict''' + '''Invoke a QMP command and return the response dict''' qmp_args = dict() - for k in args.keys(): + for key in args.keys(): if conv_keys: - qmp_args[k.translate(self.underscore_to_dash)] = args[k] + qmp_args[key.translate(self.underscore_to_dash)] = args[key] else: - qmp_args[k] = args[k] + qmp_args[key] = args[key] return self._qmp.cmd(cmd, args=qmp_args) def command(self, cmd, conv_keys=True, **args): + ''' + Invoke a QMP command. + On success return the response dict. + On failure raise an exception. + ''' reply = self.qmp(cmd, conv_keys, **args) if reply is None: raise Exception("Monitor is closed") @@ -207,7 +232,15 @@ class QEMUMachine(object): return events def event_wait(self, name, timeout=60.0, match=None): - # Test if 'match' is a recursive subset of 'event' + ''' + Wait for specified timeout on named event in QMP; optionally filter + results by match. + + The 'match' is checked to be a recursive subset of the 'event'; skips + branch processing on match's value None + {"foo": {"bar": 1}} matches {"foo": None} + {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}} + ''' def event_match(event, match=None): if match is None: return True @@ -240,4 +273,8 @@ class QEMUMachine(object): return None def get_log(self): + ''' + After self.shutdown or failed qemu execution, this returns the output + of the qemu process. + ''' return self._iolog From 2782fc517d6720dbec24b4dfa08aa4606c72c76d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:05 +0200 Subject: [PATCH 02/15] qemu|qtest: Avoid dangerous arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The list object is mutable in python and potentially might modify other object's arguments when used as default argument. Reproducer: >>> vm1 = QEMUMachine("qemu") >>> vm2 = QEMUMachine("qemu") >>> vm1._wrapper.append("foo") >>> print vm2._wrapper ['foo'] In this case the `args` is actually copied so it would be safe to keep it, but it's not a good practice to keep it. The same issue applies in inherited qtest module. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Reviewed-by: John Snow Message-Id: <20170818142613.32394-3-ldoktor@redhat.com> Reviewed-by: Cleber Rosa Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 6 +++++- scripts/qtest.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index b45e691538..afd98a290e 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -30,7 +30,7 @@ class QEMUMachine(object): # vm is guaranteed to be shut down here ''' - def __init__(self, binary, args=[], wrapper=[], name=None, + def __init__(self, binary, args=None, wrapper=None, name=None, test_dir="/var/tmp", monitor_address=None, socket_scm_helper=None, debug=False): ''' @@ -46,6 +46,10 @@ class QEMUMachine(object): @param debug: enable debug mode @note: Qemu process is not started until launch() is used. ''' + if args is None: + args = [] + if wrapper is None: + wrapper = [] if name is None: name = "qemu-%d" % os.getpid() if monitor_address is None: diff --git a/scripts/qtest.py b/scripts/qtest.py index d5aecb5f49..ab183c0635 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -79,7 +79,7 @@ class QEMUQtestProtocol(object): class QEMUQtestMachine(qemu.QEMUMachine): '''A QEMU VM''' - def __init__(self, binary, args=[], name=None, test_dir="/var/tmp", + def __init__(self, binary, args=None, name=None, test_dir="/var/tmp", socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() From 7f33ca7878e3414f779a5d89f04c68c0438c3dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:06 +0200 Subject: [PATCH 03/15] qemu.py: Use iteritems rather than keys() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's avoid creating an in-memory list of keys and query for each value and use `iteritems` which is an iterator of key-value pairs. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20170818142613.32394-4-ldoktor@redhat.com> Reviewed-by: Cleber Rosa Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index afd98a290e..d8c169b31e 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -200,11 +200,11 @@ class QEMUMachine(object): def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the response dict''' qmp_args = dict() - for key in args.keys(): + for key, value in args.iteritems(): if conv_keys: - qmp_args[key.translate(self.underscore_to_dash)] = args[key] + qmp_args[key.translate(self.underscore_to_dash)] = value else: - qmp_args[key] = args[key] + qmp_args[key] = value return self._qmp.cmd(cmd, args=qmp_args) From 41f714b190ffff7fefb3ad090bc02d089e4c7bda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:07 +0200 Subject: [PATCH 04/15] qemu.py: Simplify QMP key-conversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The QMP key conversion consist of '_'s to be replaced with '-'s, which can easily be done by a single `str.replace` method which is faster and does not require `string` module import. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Message-Id: <20170818142613.32394-5-ldoktor@redhat.com> Reviewed-by: Cleber Rosa Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index d8c169b31e..bde8da8fe7 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -13,7 +13,6 @@ # import errno -import string import os import sys import subprocess @@ -195,14 +194,12 @@ class QEMUMachine(object): self._load_io_log() self._post_shutdown() - underscore_to_dash = string.maketrans('_', '-') - def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the response dict''' qmp_args = dict() for key, value in args.iteritems(): if conv_keys: - qmp_args[key.translate(self.underscore_to_dash)] = value + qmp_args[key.replace('_', '-')] = value else: qmp_args[key] = value From a004e249f035f8a0c859d1700abb4081c86f8c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:08 +0200 Subject: [PATCH 05/15] qemu.py: Use custom exceptions rather than Exception MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The naked Exception should not be widely used. It makes sense to be a bit more specific and use better-suited custom exceptions. As a benefit we can store the full reply in the exception in case someone needs it when catching the exception. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Message-Id: <20170818142613.32394-6-ldoktor@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index bde8da8fe7..7c6802609a 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -19,6 +19,19 @@ import subprocess import qmp.qmp +class MonitorResponseError(qmp.qmp.QMPError): + ''' + Represents erroneous QMP monitor reply + ''' + def __init__(self, reply): + try: + desc = reply["error"]["desc"] + except KeyError: + desc = reply + super(MonitorResponseError, self).__init__(desc) + self.reply = reply + + class QEMUMachine(object): '''A QEMU VM @@ -213,9 +226,9 @@ class QEMUMachine(object): ''' reply = self.qmp(cmd, conv_keys, **args) if reply is None: - raise Exception("Monitor is closed") + raise qmp.qmp.QMPError("Monitor is closed") if "error" in reply: - raise Exception(reply["error"]["desc"]) + raise MonitorResponseError(reply) return reply["return"] def get_qmp_event(self, wait=False): From 9d47f6de10e36c988e935069bf288d39bdcc1126 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:09 +0200 Subject: [PATCH 06/15] qmp.py: Couple of pylint/style fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No actual code changes, just initializing attributes earlier to avoid AttributeError on early introspection, a few pylint/style fixes and docstring clarifications. Signed-off-by: Lukáš Doktor Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20170818142613.32394-7-ldoktor@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/qmp/qmp.py | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 62d3651967..782d1ac5df 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -13,19 +13,30 @@ import errno import socket import sys + class QMPError(Exception): pass + class QMPConnectError(QMPError): pass + class QMPCapabilitiesError(QMPError): pass + class QMPTimeoutError(QMPError): pass + class QEMUMonitorProtocol: + + #: Socket's error class + error = socket.error + #: Socket's timeout + timeout = socket.timeout + def __init__(self, address, server=False, debug=False): """ Create a QEMUMonitorProtocol class. @@ -42,6 +53,7 @@ class QEMUMonitorProtocol: self.__address = address self._debug = debug self.__sock = self.__get_sock() + self.__sockfile = None if server: self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) self.__sock.bind(self.__address) @@ -56,7 +68,7 @@ class QEMUMonitorProtocol: def __negotiate_capabilities(self): greeting = self.__json_read() - if greeting is None or not greeting.has_key('QMP'): + if greeting is None or "QMP" not in greeting: raise QMPConnectError # Greeting seems ok, negotiate capabilities resp = self.cmd('qmp_capabilities') @@ -78,8 +90,6 @@ class QEMUMonitorProtocol: continue return resp - error = socket.error - def __get_events(self, wait=False): """ Check for new events in the stream and cache them in __events. @@ -89,8 +99,8 @@ class QEMUMonitorProtocol: @raise QMPTimeoutError: If a timeout float is provided and the timeout period elapses. - @raise QMPConnectError: If wait is True but no events could be retrieved - or if some other error occurred. + @raise QMPConnectError: If wait is True but no events could be + retrieved or if some other error occurred. """ # Check for new events regardless and pull them into the cache: @@ -175,7 +185,7 @@ class QEMUMonitorProtocol: @param args: command arguments (dict) @param id: command id (dict, list, string or int) """ - qmp_cmd = { 'execute': name } + qmp_cmd = {'execute': name} if args: qmp_cmd['arguments'] = args if id: @@ -183,6 +193,9 @@ class QEMUMonitorProtocol: return self.cmd_obj(qmp_cmd) def command(self, cmd, **kwds): + """ + Build and send a QMP command to the monitor, report errors if any + """ ret = self.cmd(cmd, kwds) if ret.has_key('error'): raise Exception(ret['error']['desc']) @@ -190,15 +203,15 @@ class QEMUMonitorProtocol: def pull_event(self, wait=False): """ - Get and delete the first available QMP event. + Pulls a single event. @param wait (bool): block until an event is available. @param wait (float): If wait is a float, treat it as a timeout value. @raise QMPTimeoutError: If a timeout float is provided and the timeout period elapses. - @raise QMPConnectError: If wait is True but no events could be retrieved - or if some other error occurred. + @raise QMPConnectError: If wait is True but no events could be + retrieved or if some other error occurred. @return The first available QMP event, or None. """ @@ -217,8 +230,8 @@ class QEMUMonitorProtocol: @raise QMPTimeoutError: If a timeout float is provided and the timeout period elapses. - @raise QMPConnectError: If wait is True but no events could be retrieved - or if some other error occurred. + @raise QMPConnectError: If wait is True but no events could be + retrieved or if some other error occurred. @return The list of available QMP events. """ @@ -235,8 +248,6 @@ class QEMUMonitorProtocol: self.__sock.close() self.__sockfile.close() - timeout = socket.timeout - def settimeout(self, timeout): self.__sock.settimeout(timeout) From 3dd29b4133e4f6ad595bcd835fc0b3302a3304fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:10 +0200 Subject: [PATCH 07/15] qmp.py: Use object-based class for QEMUMonitorProtocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no need to define QEMUMonitorProtocol as old-style class. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Message-Id: <20170818142613.32394-8-ldoktor@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/qmp/qmp-shell | 4 ++-- scripts/qmp/qmp.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index 860ffb27f2..be449de621 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -106,7 +106,7 @@ class FuzzyJSON(ast.NodeTransformer): # _execute_cmd()). Let's design a better one. class QMPShell(qmp.QEMUMonitorProtocol): def __init__(self, address, pretty=False): - qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address)) + super(QMPShell, self).__init__(self.__get_address(address)) self._greeting = None self._completer = None self._pretty = pretty @@ -281,7 +281,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): return True def connect(self, negotiate): - self._greeting = qmp.QEMUMonitorProtocol.connect(self, negotiate) + self._greeting = super(QMPShell, self).connect(negotiate) self.__completer_setup() def show_banner(self, msg='Welcome to the QMP low-level shell!'): diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 782d1ac5df..95ff5cc39a 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError): pass -class QEMUMonitorProtocol: +class QEMUMonitorProtocol(object): #: Socket's error class error = socket.error From 2cb05a3f3613f633b916b854cfaf04c663ccc953 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:11 +0200 Subject: [PATCH 08/15] qmp.py: Avoid "has_key" usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "has_key" is deprecated in favor of "__in__" operator. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20170818142613.32394-9-ldoktor@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/qmp/qmp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 95ff5cc39a..f2f5a9b296 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object): Build and send a QMP command to the monitor, report errors if any """ ret = self.cmd(cmd, kwds) - if ret.has_key('error'): + if "error" in ret: raise Exception(ret['error']['desc']) return ret['return'] From 7b6b9dbb3c4cb7eac4c847d601735da9b1d150ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:12 +0200 Subject: [PATCH 09/15] qmp.py: Avoid overriding a builtin object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "id" is a builtin method to get object's identity and should not be overridden. This might bring some issues in case someone was directly calling "cmd(..., id=id)" but I haven't found such usage on brief search for "cmd\(.*id=". Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Message-Id: <20170818142613.32394-10-ldoktor@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/qmp/qmp.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index f2f5a9b296..ef12e8a1a0 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object): print >>sys.stderr, "QMP:<<< %s" % resp return resp - def cmd(self, name, args=None, id=None): + def cmd(self, name, args=None, cmd_id=None): """ Build a QMP command and send it to the QMP Monitor. @param name: command name (string) @param args: command arguments (dict) - @param id: command id (dict, list, string or int) + @param cmd_id: command id (dict, list, string or int) """ qmp_cmd = {'execute': name} if args: qmp_cmd['arguments'] = args - if id: - qmp_cmd['id'] = id + if cmd_id: + qmp_cmd['id'] = cmd_id return self.cmd_obj(qmp_cmd) def command(self, cmd, **kwds): From 4d9342977a2e8b195609d332fcd64a93a48c158b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Doktor?= Date: Fri, 18 Aug 2017 16:26:13 +0200 Subject: [PATCH 10/15] qtest.py: Few pylint/style fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No actual code changes, just few pylint/style fixes. Signed-off-by: Lukáš Doktor Reviewed-by: John Snow Message-Id: <20170818142613.32394-11-ldoktor@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/qtest.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/scripts/qtest.py b/scripts/qtest.py index ab183c0635..df0daf26ca 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -11,14 +11,11 @@ # Based on qmp.py. # -import errno import socket -import string import os -import subprocess -import qmp.qmp import qemu + class QEMUQtestProtocol(object): def __init__(self, address, server=False): """ @@ -83,8 +80,10 @@ class QEMUQtestMachine(qemu.QEMUMachine): socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() - super(QEMUQtestMachine, self).__init__(binary, args, name=name, test_dir=test_dir, - socket_scm_helper=socket_scm_helper) + super(QEMUQtestMachine, + self).__init__(binary, args, name=name, test_dir=test_dir, + socket_scm_helper=socket_scm_helper) + self._qtest = None self._qtest_path = os.path.join(test_dir, name + "-qtest.sock") def _base_args(self): From f6cf7f5a227ca0c8cc540d78d4f0f943c51ea8d1 Mon Sep 17 00:00:00 2001 From: Amador Pahim Date: Fri, 1 Sep 2017 13:28:17 +0200 Subject: [PATCH 11/15] qemu.py: fix is_running() return before first launch() is_running() returns None when called before the first time we call launch(): >>> import qemu >>> vm = qemu.QEMUMachine('qemu-system-x86_64') >>> vm.is_running() >>> It should return False instead. This patch fixes that. For consistence, this patch removes the parenthesis from the second clause as it's not really needed. Signed-off-by: Amador Pahim Message-Id: <20170901112829.2571-2-apahim@redhat.com> Reviewed-by: Fam Zheng Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 7c6802609a..f80b008f7f 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -131,7 +131,7 @@ class QEMUMachine(object): raise def is_running(self): - return self._popen and (self._popen.returncode is None) + return self._popen is not None and self._popen.returncode is None def exitcode(self): if self._popen is None: From 4738b0a85a0c2031fddc71b51cccebce0c4ba6b1 Mon Sep 17 00:00:00 2001 From: Amador Pahim Date: Fri, 1 Sep 2017 13:28:18 +0200 Subject: [PATCH 12/15] qemu.py: avoid writing to stdout/stderr This module should not write directly to stdout/stderr. Instead, it should either raise exceptions or just log the messages and let the callers handle them and decide what to do. For example, scripts could choose to send the log messages stderr or/and write them to a file if verbose or debugging mode is enabled. This patch replaces the writes to stderr by an exception in the send_fd_scm() when _socket_scm_helper is not set or not present. In the same method, the subprocess Popen will now redirect the stdout/stderr to logging.debug instead of writing to system stderr. As consequence, since the Popen.communicate() is now used (in order to get the stdout), the further call to wait() became redundant and was replaced by Popen.returncode. The shutdown() message on negative exit code will now be logged to logging.warn instead of written to system stderr. Signed-off-by: Amador Pahim Message-Id: <20170901112829.2571-3-apahim@redhat.com> Reviewed-by: Fam Zheng Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index f80b008f7f..8d3d24dd2b 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -13,12 +13,22 @@ # import errno +import logging import os import sys import subprocess import qmp.qmp +LOG = logging.getLogger(__name__) + + +class QEMUMachineError(Exception): + """ + Exception called when an error in QEMUMachine happens. + """ + + class MonitorResponseError(qmp.qmp.QMPError): ''' Represents erroneous QMP monitor reply @@ -107,18 +117,21 @@ class QEMUMachine(object): # In iotest.py, the qmp should always use unix socket. assert self._qmp.is_scm_available() if self._socket_scm_helper is None: - print >>sys.stderr, "No path to socket_scm_helper set" - return -1 + raise QEMUMachineError("No path to socket_scm_helper set") if not os.path.exists(self._socket_scm_helper): - print >>sys.stderr, "%s does not exist" % self._socket_scm_helper - return -1 + raise QEMUMachineError("%s does not exist" % + self._socket_scm_helper) fd_param = ["%s" % self._socket_scm_helper, "%d" % self._qmp.get_sock_fd(), "%s" % fd_file_path] devnull = open('/dev/null', 'rb') - proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout, - stderr=sys.stderr) - return proc.wait() + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) + output = proc.communicate()[0] + if output: + LOG.debug(output) + + return proc.returncode @staticmethod def _remove_if_exists(path): @@ -202,8 +215,8 @@ class QEMUMachine(object): exitcode = self._popen.wait() if exitcode < 0: - sys.stderr.write('qemu received signal %i: %s\n' - % (-exitcode, ' '.join(self._args))) + LOG.warn('qemu received signal %i: %s', -exitcode, + ' '.join(self._args)) self._load_io_log() self._post_shutdown() From 63e0ba55222476c8b96d8c45131c830e00a80eaa Mon Sep 17 00:00:00 2001 From: Amador Pahim Date: Fri, 1 Sep 2017 13:28:19 +0200 Subject: [PATCH 13/15] qemu.py: use os.path.null instead of /dev/null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For increased portability, let's use os.path.devnull. Signed-off-by: Amador Pahim Message-Id: <20170901112829.2571-4-apahim@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Fam Zheng Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 8d3d24dd2b..c9bcaafe41 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -124,7 +124,7 @@ class QEMUMachine(object): fd_param = ["%s" % self._socket_scm_helper, "%d" % self._qmp.get_sock_fd(), "%s" % fd_file_path] - devnull = open('/dev/null', 'rb') + devnull = open(os.path.devnull, 'rb') proc = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) output = proc.communicate()[0] @@ -186,7 +186,7 @@ class QEMUMachine(object): def launch(self): '''Launch the VM and establish a QMP connection''' - devnull = open('/dev/null', 'rb') + devnull = open(os.path.devnull, 'rb') qemulog = open(self._qemu_log_path, 'wb') try: self._pre_launch() From dab91d9aa00e41ee680524e4f6e99a1e7fe12eb2 Mon Sep 17 00:00:00 2001 From: Amador Pahim Date: Fri, 1 Sep 2017 13:28:20 +0200 Subject: [PATCH 14/15] qemu.py: improve message on negative exit code The current message shows 'self._args', which contains only part of the options used in the Qemu command line. This patch makes the qemu full args list an instance variable and then uses it in the negative exit code message. Message was moved outside the 'if is_running' block to make sure it will be logged if the VM finishes before the call to shutdown(). Signed-off-by: Amador Pahim Message-Id: <20170901112829.2571-5-apahim@redhat.com> [ehabkost: removed superfluous parenthesis] Reviewed-by: Fam Zheng Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index c9bcaafe41..9440261ac3 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -87,6 +87,7 @@ class QEMUMachine(object): self._socket_scm_helper = socket_scm_helper self._debug = debug self._qmp = None + self._qemu_full_args = None def __enter__(self): return self @@ -186,13 +187,16 @@ class QEMUMachine(object): def launch(self): '''Launch the VM and establish a QMP connection''' + self._qemu_full_args = None devnull = open(os.path.devnull, 'rb') qemulog = open(self._qemu_log_path, 'wb') try: self._pre_launch() - args = (self._wrapper + [self._binary] + self._base_args() + - self._args) - self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog, + self._qemu_full_args = self._wrapper + [self._binary] + + self._base_args() + self._args + self._popen = subprocess.Popen(self._qemu_full_args, + stdin=devnull, + stdout=qemulog, stderr=subprocess.STDOUT, shell=False) self._post_launch() @@ -212,14 +216,20 @@ class QEMUMachine(object): self._qmp.close() except: self._popen.kill() + self._popen.wait() - exitcode = self._popen.wait() - if exitcode < 0: - LOG.warn('qemu received signal %i: %s', -exitcode, - ' '.join(self._args)) self._load_io_log() self._post_shutdown() + exitcode = self.exitcode() + if exitcode is not None and exitcode < 0: + msg = 'qemu received signal %i: %s' + if self._qemu_full_args: + command = ' '.join(self._qemu_full_args) + else: + command = '' + LOG.warn(msg, exitcode, command) + def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the response dict''' qmp_args = dict() From b92a0011b1220aff549ae82c6104014d25e339ef Mon Sep 17 00:00:00 2001 From: Amador Pahim Date: Fri, 1 Sep 2017 13:28:21 +0200 Subject: [PATCH 15/15] qemu.py: include debug information on launch error When launching a VM, if an exception happens and the VM is not initiated, it might be useful to see the qemu command line and the qemu command output. This patch creates that message. Notice that self._iolog needs to be cleaned up in the beginning of the launch() to make sure we will not expose the qemu log from a previous launch if the current one fails. Signed-off-by: Amador Pahim Message-Id: <20170901112829.2571-6-apahim@redhat.com> Reviewed-by: Fam Zheng Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/qemu.py b/scripts/qemu.py index 9440261ac3..8c67595ec8 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -187,6 +187,7 @@ class QEMUMachine(object): def launch(self): '''Launch the VM and establish a QMP connection''' + self._iolog = None self._qemu_full_args = None devnull = open(os.path.devnull, 'rb') qemulog = open(self._qemu_log_path, 'wb') @@ -206,6 +207,12 @@ class QEMUMachine(object): self._popen.wait() self._load_io_log() self._post_shutdown() + + LOG.debug('Error launching VM') + if self._qemu_full_args: + LOG.debug('Command: %r', ' '.join(self._qemu_full_args)) + if self._iolog: + LOG.debug('Output: %r', self._iolog) raise def shutdown(self):